Incidentally, please do not hijack existing threads when you start a new
topic, ok?

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:
> > 
> >
> > 
> > My (temporary) fork is here:
> > 
> >
> > 
> > And for this I needed a hierarchical tree-like vocabulary.
> > 
> > So I've created a TreeVocabulary in zope/schema/, 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:
> >
> > 
> > The only changes are the new TreeVocabulary in zope/schema/
> > and the tests for it in zope/schema/tests/
> > 
> > Can someone please take a look and give some feedback?, 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., line 150:

  "gne or more interfaces may also be provided so that alternate"

s/gne/One/, line 223:

  self.assertTrue(dict, type(v._terms)) 
s/assertTrue/assertEqual/, line 249:

  """ len returns the number of all nodes in die dict 


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.

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

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 think TreeVocabulary should have a corresponding interface

The new getTermPath() method is currently undocumented.

What's the use case for a tree vocabulary?  A widget that displays the
tree structure explicitly?  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'm unhappy about len(tree_vocab) !+ len(list(tree_vocab)).

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

> Should I rather create a ticket on launchpad? Or otherwise, should I
> just go ahead and merge the changes to trunk?

Marius Gedminas
-- -- Zope 3/BlueBream consulting and development

Attachment: signature.asc
Description: Digital signature

Zope-Dev maillist  -
**  No cross posts or HTML encoding!  **
(Related lists - )

Reply via email to