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