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