On Mon, 2012-01-30 at 15:42 +0100, Charlie Clark wrote: > Am 30.01.2012, 14:33 Uhr, schrieb Jan-Carel Brand <li...@opkode.com>: > > ... lots cut ... > > > 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. > > I think the tests should be extended to show that this also includes > nesting because this is non-obvious. ie. I believe that the region "Izmir" > is within the state of "Izimir" in Turkey.
Ok, I've added the method *test_nonunique_values_and_tokens* to the tests. > >> 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. > > hm, okay. Personally, I think you should be able to populate your > dictionaries with only a single pass through the terms. However, as this > only needs to happen when the application starts we don't need to worry > too much about the performance. I think you're right, so I've refactored _flattenTree and _getPathIndex into a single method _populateIndexes that populates all three indexes with one tree-traversal. > > 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. > > Then someone will speak up for it if they need it or do their own > subclassing/composition as required. Otherwise it's just food for warts. > >> 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. > > Well, we could always think about removing it from SimpleVocabulary: it's > not in the interface so no subclass actually has the right to depend on > it. ;-) Looking at the documentation in fields.txt I see the following: > A vocabulary must minimally be able to determine whether it contains a > value, to create a term object for a value, and to return a query > interface (or None) to find items in itself. > So it looks like someone thinks that any vocabulary must be able to create a term object. Thoughts on that? I think I'm now finished and have implemented everybody's suggestions and improvements. The TreeVocabulary now has a terms_factory attribute which is an OrderedDict by default and which can be overridden in a subclass to an alternative datastructure. It also implements IEnumerableMapping. All the TreeVocabulary methods are covered by tests and I tested with Python 2.6 and Python 2.7. If there aren't any more objections, I'd like to now merge with trunk. Regards J-C > > _______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org 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 )