Re: TreeVocabulary in zope.schema.vocabulary

On Sun, 2012-01-29 at 16:17 +0100, Charlie Clark wrote:

<snip>

> > From the zope.schema 4.0.0 release notes:
> > "Port to Python 3. This adds a dependency on six and removes support for
> > Python 2.5"
> 
> Shouldn't that also mean a preference of
> "for key in _dict" over "for key in _dict.keys()" ?
> 
> Though you might prefer .items() in _getPathToTreeNode()
> 
> i.e
>     def _getPathToTreeNode(self, tree, node)
> 
>          path = []
>          for parent, child in tree.items():
>              if node == parent.value:
>                  return [node]
>              path = self._getPathToTreeNode(child, node)
>              if path:
>                  path.insert(0, parent.value)
>                  break
>          return path

Thanks, I've taken this code from the diff file you gave.

> > So if we want to use OrderedDict (which is from Python >= 2.7), we just
> > need to bridge the Python 2.6 version with the ordereddict package:
> > http://pypi.python.org/pypi/ordereddict/1.1
> > This would introduce a new dependency for zope.schema on Python 2.6, I
> > don't know if that's acceptable or not.
> 
> I think it's perfectly justified in this case and similar to what has  
> happened with other libraries like ElementTree in the past that have made  
> life easier and subsequently been adopted by the standard library.

Marius and Souheil, what do you think about this? I think it probably
still makes sense to have a dict_factory as Souheil suggested, but then
we make the default type OrderedDict and not dict.

Any objections?

> > In setup.py one could specify the extra dependency only for Python <
> > 2.7:
> >
> >> import sys
> >>
> >> REQUIRES = ['setuptools',
> >> 	    'zope.interface >= 3.6.0',
> >> 	    'zope.event',
> >> 	    'six',
> >> 	   ]
> >>
> >> if sys.version_info < (2 , 7):
> >>     REQUIRES += ['ordereddict'],
> >>
> >> setup(
> >> # [...]
> >>     install_requires=REQUIRES,
> >> # [...]
> 
> Yep.

<snip>

> > I however don't see how one could use a generator for the recursive
> > methods that return dictionaries.
> > With regards to the "recurse" method (now called _getPathToTreeNode), I
> > don't see how one could use a generator in a more efficient manner than
> > the normal recursion that's being used currently.
> > I played around with it and the best I could come up with is this:
> >        def generator(_dict, value, path=[]):
> >             if value in _dict.keys():
> >                 yield path+[value]
> >            for key in _dict.keys():
> >                 for path in recurse(_dict[key], value, path+[key]):
> >                     if value in path:
> >                         yield path
> > You still have to recurse through the different branches until you find
> > the node that you are looking for and you still need to store the path
> > in a list. So what would be the added value?
> > What's more, the generator returns a list within a list, like so:
> > [['Regions', 'Germany', 'Bavaria']], which I find clunky.
> 
> You're probably right. I think I was getting ahead of myself wondering  
> about possible issues with deeply nested vocabularies. 

> Any real improvement would probably involve a node index. 
> I notice that the examples do not allow for departments in regions to have the same name as  
> the region (common enough in some countries) so you could simply add the  
> index keyed by node value with the path as the value when the class is  
> created. 

Yes, the values must be unique, because we do value lookups. 

The "title" attr doesn't have to be unique though. You could have a
"Forestry" department for two different regions. The title will be the
same, but the value and token attrs can't.

> This should be possible by calling _getPathToTreeNode during one  
> of the passes through _flattenTree. getTermPath would then just need to do  
> a lookup on this.

I don't like the way the path_node gets implicitly populated during a
call to _flattenTree. 

I'd rather have a separate method that calculates the path and then
explicitly assign it to self.path_node.

In any case, there is now a node_index in the code :)

<snip>

> >> but I don't see the advantage of
> >> cls.createTerm(*args) over SimpleTerm(*args)
> > See above. "createTerm" is there to let developers override it and
> > provide their own term objects.
> 
> Do you have a concrete use case for this? 

Not really, but that doesn't mean it doesn't exist.

> Remember that createTerm is a  
> convenience method only. Frankly, I don't see the need for it in what is a  
> fairly specialised class.

I like consistency and symmetry, so if SimpleVocabulary has it, as an
add-on developer I'd expect for TreeVocabulary to also have it.

I don't however feel very strongly about it though, and I wanna get this
done, so I removed it.

J-C

_______________________________________________
Zope-Dev maillist  -  [email protected]
https://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
 https://mail.zope.org/mailman/listinfo/zope-announce
 https://mail.zope.org/mailman/listinfo/zope )