Hi Souheil On Wed, 2012-01-25 at 18:56 +0100, Souheil CHELFOUH wrote:
A quick note :
This quite an advanced vocabulary, why not make another package with a dependency on zope.schema ? I don't quite see the point to have that in the "core".
Ok, Charlie also expressed his reservations. I'll put it in a different package then. I'm not too sure what to name it though. For example, under what namespace? zope or z3c? I'm guessing zope.vocabulary, or rather zope.treevocabulary?
Furthermore, for the dict class in use in the vocabulary, you could add a "factory" class that can be overriden easily. That would allow people with OrderDict capabilities to use them without having to re-sort later on.
Could you please elaborate on what you mean? If I create a factory class to create TreeVocabulary instances, how will overriding that factory (without creating a separate SortableTreeVocabulary) allow people to use OrderedDict? Incidentally, I came upon this: http://pypi.python.org/pypi/ordereddict which provides the OrderedDict to Python 2.4 to 2.7 I think it might make sense to just subclass OrderedDict and implement an ordered tree from the start.
By the way, good work on that, it's something that is often needed in advanced forms. I'll make sure to try it. Thank you for the effort.
Thanks! Much appreciated. J-C
- Souheil
2012/1/25 Marius Gedminas <marius@gedmin.as>:
On Wed, Jan 25, 2012 at 01:55:28AM +0200, Jan-Carel Brand wrote:
On Wed, 2012-01-25 at 00:52 +0200, Marius Gedminas wrote:
On Tue, Jan 24, 2012 at 07:34:03PM +0200, Jan-Carel Brand wrote:
I now subclass PersistentMapping instead of SimpleVocabulary, so this is not an issue anymore.
Ok. But why Persistent? None of the other vocabularies are persistent...
Yeah, using PersistentMapping was a mistake, firstly because persistence is not necessary and secondly because it introduces a dependency on Persistence. <...>
I've changed the TreeVocabulary to subclass from PersistentDict. So the vocabulary itself now acts as a dict.
So is it PersistentMapping or PersistentDict then? ;)
It was first the one, and then the other :)
For extra fun: one is an alias for the other in newer ZODB versions.
> 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?
Thank you, yes.
I'm still wondering about the possibility of ordered trees.
Python 2.7 has an OrderedDict class in the collections module: http://docs.python.org/dev/whatsnew/2.7.html#pep-0372
Yes. And if I pass an OrderedDict to your .fromDict(), it will be discarded and all the items inserted into a regular dict, forgetting their original order.
But anyway, I'm fine with you saying "explicit ordering is not supported; it's up to the widget to sort each tree level appropriately". In the class docstring, say. ;-)
And I'm -1 for subclassing PersistentMapping. It may tempt people into storing tree vocabularies in the ZODB, and then maybe even modifying them. And you have plenty of non-persistent dicts in the internal structure.
I think it would be better to subclass a regular dict, and document that you ITreeVocabulary is a dict-like object by making it inherit IEnumerableMapping.
Thanks for the suggestion, I did that.
I've no objections remaining (other than that little thing about explicit ordering).
Marius Gedminas -- http://pov.lt/ -- Zope 3/BlueBream consulting and development
_______________________________________________ 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 )
_______________________________________________ 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 )