Re: TreeVocabulary in zope.schema.vocabulary

On Tue, 2012-01-24 at 16:07 +0200, Marius Gedminas wrote:
> Incidentally, please do not hijack existing threads when you start a new
> topic, ok?

Yes, that was an honest mistake, no ill intentions. Won't happen again.


> On Tue, Jan 24, 2012 at 01:35:49PM +0200, Jan-Carel Brand wrote:
> > On Fri, 2012-01-20 at 13:50 +0200, Jan-Carel Brand wrote:
> > > Hi all
> > > 
> > > I've been working on porting the Dynatree (a dynamic tree-like) widget
> > > to z3c.form:
> > > 
> > > https://github.com/collective/collective.dynatree
> > > 
> > > My (temporary) fork is here:
> > > 
> > > https://github.com/syslabcom/collective.dynatree
> > > 
> > > And for this I needed a hierarchical tree-like vocabulary.
> > > 
> > > So I've created a TreeVocabulary in zope/schema/vocabulary.py, based
> > > upon the existing SimpleVocabulary.
> > > 
> > > Instead of fromValues or fromItems, it has fromDict, to construct the
> > > vocab from a dict. And the internal representation, self._terms, is a
> > > dictionary.
> > > 
> > > My branch is here:
> > > http://svn.zope.org/zope.schema/branches/jcbrand-treevocabulary/
> > > 
> > > The only changes are the new TreeVocabulary in zope/schema/vocabulary.py
> > > and the tests for it in zope/schema/tests/test_vocabulary.py
> > > 
> > > Can someone please take a look and give some feedback? 
> 
> vocabulary.py, line 146:
> 
>   """Initialize the vocabulary given a dict of terms.
> 
> Please clarify what a 'dict of terms' means.  AFAIC the *keys* of your
> dict are terms (instances of SimpleTerm or whatever), and the values are
> dicts representing the children.

Ok, I've clarified that a bit more.

> vocabulary.py, line 150:
> 
>   "gne or more interfaces may also be provided so that alternate"
> 
> s/gne/One/

Fixed

> test_vocabulary.py, line 223:
> 
>   self.assertTrue(dict, type(v._terms)) 
>   
> s/assertTrue/assertEqual/

Fixed.

> test_vocabulary.py, line 249:
> 
>   """ len returns the number of all nodes in die dict 
> 
> s/die/the/

Fixed.

> lines 255-257:
> 
>   self.assertTrue('Regions' in self.tree_vocab_2 and \ 
>                   'Austria' in self.tree_vocab_2 and \ 
>                   'Bavaria' in self.tree_vocab_2) 
> 
> The backslashes at the end of each line are not necessary.  Same on
> lines 262-264.

Removed.

> test_vocabulary, lines 192-194: you create list_vocab and items_vocab
> and then never use them.

Removed.

> Missing tests: by inheriting from SimpleVocabulary you also gain
> .fromItems() and .fromValues().  Do those work?  They pass a list of
> terms to __init__, which seems to expect a dict now.  Override and add a
> raise NotImplementedError?  Or just make them work?

I now subclass PersistentMapping instead of SimpleVocabulary, so this is
not an issue anymore.

> I think TreeVocabulary should have a corresponding interface
> ITreeVocabulary.

I agree, done.

> The new getTermPath() method is currently undocumented.

I added documentation.

> What's the use case for a tree vocabulary?  A widget that displays the
> tree structure explicitly?  

Yes. In my case, it's for the widget in collective.dynatree. This is a
fairly common use-case in Plone. Products.ATVocabularyManager also has
hierarchical vocabularies.

> It seems... difficult to extract that
> tree structure using just the public API.  Actually, it's impossible:
> __iter__ doesn't return all the terms, just top-level ones.  Am I
> missing something?

I've changed the TreeVocabulary to subclass from PersistentDict. So the
vocabulary itself now acts as a dict.

> I'm unhappy about len(tree_vocab) !+ len(list(tree_vocab)).

You're right. I fixed that.

> > Perhaps I should rephrase :)
> > 
> > I would like my changes to be merged with the zope.schema trunk. The
> > tests I've added provide 100% coverage of the TreeVocabulary code.
> > 
> > I would just like someone to sign it off.
> 
> -1 because of the concerns above.

Fair enough. Have your concerns been addressed properly?

Thanks Marius, and Charlie, for taking the time to check the code. I
appreciate it!


JC

_______________________________________________
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 )