Re: TreeVocabulary in zope.schema.vocabulary

On Mon, 2012-01-30 at 15:42 +0100, Charlie Clark wrote:
> Am 30.01.2012, 14:33 Uhr, schrieb Jan-Carel Brand <[email protected]>:
> 
> ... 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  -  [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 )