[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
> 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/
> 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.
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.
> 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
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)
> @@ -0,0 +1,5 @@
> +<configure xmlns="http://namespaces.zope.org/zope">
> + <include package=".generations" />
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._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
The Plone IExplicitOrdering interface looks reasonable, and could
really take advantage of the blist characteristics.
> 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.
Zope-Dev maillist - Zope-Dev@zope.org
** No cross posts or HTML encoding! **
(Related lists -