[Zope-dev] Review of zc.dict tlotze-blist branch

[Gary Poster]

[Thomas asked me to review his zc.dict branch a while ago.]

Hi Thomas.  Thank you for this work.  It looks great.  I do have  
several comments below (from an abbreviated diff against the current  
trunk).

 > Index: buildout.cfg
 > ===================================================================
 > --- buildout.cfg	(.../trunk)	(revision 97211)
 > +++ buildout.cfg	(.../branches/tlotze-blist)	(revision 97211)
 > @@ -2,11 +2,10 @@
 >  develop = .
 >  parts = test py
 >
 > -find-links = http://download.zope.org/distribution/
 > -
 >  [test]
 >  recipe = zc.recipe.testrunner
 > -eggs = zc.dict
 > +eggs = zc.dict [generations]
 > +defaults = ["-v", "-c"]

I don't think the generations code should be required, and (since you  
used extras_require) neither do you.  Therefore I'd prefer it if the  
main tests also didn't depend on that code.  The way I've done this  
with other packages (e.g. http://svn.zope.org/zc.async/trunk/buildout.cfg?view=auto) 
  is to have multiple test sections (with different names).  The  
downside is that then, to run all possible tests, you have to run more  
than one command.  The upside is that arguably the most important  
configuration--the base one--is truly tested.

 >
 >  [py]
 >  recipe = zc.recipe.egg
 > Index: CHANGES.txt
 > ===================================================================
 > --- CHANGES.txt	(.../trunk)	(revision 97211)
 > +++ CHANGES.txt	(.../branches/tlotze-blist)	(revision 97211)
 > @@ -1,3 +1,8 @@
 > +1.3 (unreleased)
 > +----------------
 > +
 > +* changed the implementation of OrderedDict to store the order in  
buckets

I suggest adding "via zc.blist" or something like that.

...

 > Index: src/zc/dict/configure.zcml
 > ===================================================================
 > --- src/zc/dict/configure.zcml	(.../trunk)	(revision 0)
 > +++ src/zc/dict/configure.zcml	(.../branches/tlotze-blist)	 
(revision 97211)
 > @@ -0,0 +1,5 @@
 > +<configure xmlns="http://namespaces.zope.org/zope">
 > +
 > +  <include package=".generations" />
 > +
 > +</configure>

configure.zcml has a semantic of "always include."  Because the  
generations code may not work for many people (as we've discussed  
before, and see comment in evolve test below), I'd prefer that the  
generations code have a semantic of "optionally include."  Therefore,  
I suggest you rename this to "generations.zcml" or something like that.

...

 > Index: src/zc/dict/dict.py
 > ===================================================================
 > --- src/zc/dict/dict.py	(.../trunk)	(revision 97211)
 > +++ src/zc/dict/dict.py	(.../branches/tlotze-blist)	(revision 97211)

...

 >      def __setitem__(self, key, value):
 > -        delta = 1
 > -        if key in self._data:
 > -            delta = 0
 > -        self._data[key] = value
 > -        if delta:
 > +        if key not in self._data:
 >              self._order.append(key)
 > -            self._len.change(delta)
 > +            self._len.change(1)
 > +        self._data[key] = value

Nice improvement in readability.

 >
 >      def updateOrder(self, order):

...

Also as mentioned before on the Zope list, until this API is  
deprecated in favor of one that encourages more granular changes, the  
change to blist only really helps the story for adding new items to an  
ordered dict.

The Plone IExplicitOrdering interface looks reasonable, and could  
really take advantage of the blist characteristics.

http://dev.plone.org/plone/browser/plone.folder/trunk/plone/folder/interfaces.py

 > Index: src/zc/dict/generations/evolve1.txt
 > ===================================================================
 > --- src/zc/dict/generations/evolve1.txt	(.../trunk)	(revision 0)
 > +++ src/zc/dict/generations/evolve1.txt	(.../branches/tlotze-blist)	

...

As we discussed earlier, findObjectsMatching will miss OrderedDicts  
that are used as internal data structures.  I regard that as a primary  
use case for this package, so IMO that's important to note in the doc  
and in the Python file.

Otherwise, looks great to me.

Thank you!

Gary