zope.app.server emits a ProcessStarting event on startup.
zope.app.wsgi.paste doesn't. I think it should. If there are no objections, I'll make a bug fix release for this.
BTW, I ended up making a 3.9.3zc1 release (this will spur 3.9.3zc2 release) because the latest release breaks our apps' tests. Not sure if this was due to zope.testbrowser 4, or the (until recently) unreleased zope.app.testing changes needed to work with zope.testbrowser 4. I haven't had time to chase these down, thus the 3.9.3zc1 release, which I'm not terribly proud of.
Jim
-- Jim Fulton http://www.linkedin.com/in/jimfulton
On Wed, Jan 18, 2012 at 05:00:27PM -0500, Jim Fulton wrote:
zope.app.server emits a ProcessStarting event on startup.
zope.app.wsgi.paste doesn't. I think it should.
+1
If there are no objections, I'll make a bug fix release for this.
Hm. I've an app that wraps zope.app.wsgi like this:
from zope.app.wsgi import getWSGIApplication from zope.app.appsetup.interfaces import ProcessStarting from zope.event import notify
def app_factory(global_config, **local_conf): """Create a Zope WSGI application given a zope configuration.
The configuration needs to be specified as the 'config' parameter, e.g. in paster.ini use::
[app:main] use = egg:ivija config = site-definition %(site-definition)s <zodb> <zeoclient> server %(run-directory)s/zeosock </zeoclient> </zodb> <eventlog> <logfile> formatter zope.exceptions.log.Formatter path %(log-directory)s/z3.log </logfile> <logfile> formatter zope.exceptions.log.Formatter path STDOUT </logfile> </eventlog>
""" app = getWSGIApplication(StringIO(local_conf['config'])) notify(ProcessStarting()) return app
Would this result in duplicate ProcessStarting() events after your proposed change? Can duplicate ProcessStarting() events cause harm? If so, I'd rather see a bigger version bump than just a bugfix.
(We are pinning all our dependency versions with buildout-versions, so our app won't break if you rele--oh, I see you already released 3.9.3zc2 with the change.)
BTW, I ended up making a 3.9.3zc1 release (this will spur 3.9.3zc2 release) because the latest release breaks our apps' tests. Not sure if this was due to zope.testbrowser 4, or the (until recently) unreleased zope.app.testing changes needed to work with zope.testbrowser 4. I haven't had time to chase these down, thus the 3.9.3zc1 release, which I'm not terribly proud of.
I'm confused about this. "Latest release" refers to what, 3.9.3? or was there a newer version that is now hidden on PyPI?
Marius Gedminas
Am 19.01.2012 um 00:50 schrieb Marius Gedminas: [...]
I'm confused about this. "Latest release" refers to what, 3.9.3? or was there a newer version that is now hidden on PyPI?
Fixed, I unhid the 3.14.0 release additionally to the 3.9.3zc2 one.
Yours sincerely,
On Wed, Jan 18, 2012 at 6:50 PM, Marius Gedminas marius@gedmin.as wrote:
On Wed, Jan 18, 2012 at 05:00:27PM -0500, Jim Fulton wrote:
zope.app.server emits a ProcessStarting event on startup.
zope.app.wsgi.paste doesn't. I think it should.
+1
If there are no objections, I'll make a bug fix release for this.
Hm. I've an app that wraps zope.app.wsgi like this:
from zope.app.wsgi import getWSGIApplication from zope.app.appsetup.interfaces import ProcessStarting from zope.event import notify
def app_factory(global_config, **local_conf): """Create a Zope WSGI application given a zope configuration.
...
""" app = getWSGIApplication(StringIO(local_conf['config'])) notify(ProcessStarting()) return appWould this result in duplicate ProcessStarting() events after your proposed change?
Yes.
Can duplicate ProcessStarting() events cause harm?
I don't know. Theoretically.
If so, I'd rather see a bigger version bump than just a bugfix.
Ideally, but ...
(We are pinning all our dependency versions with buildout-versions, so our app won't break if you rele--oh, I see you already released 3.9.3zc2 with the change.)
Yup, yup.
BTW, I ended up making a 3.9.3zc1 release (this will spur 3.9.3zc2 release) because the latest release breaks our apps' tests. Not sure if this was due to zope.testbrowser 4, or the (until recently) unreleased zope.app.testing changes needed to work with zope.testbrowser 4. I haven't had time to chase these down, thus the 3.9.3zc1 release, which I'm not terribly proud of.
I'm confused about this. "Latest release" refers to what, 3.9.3? or was there a newer version that is now hidden on PyPI?
Sorry, my bad. I forgot to unhide 3.14.0 (and hide 3.9.3zc2). Michael Howitz fixed that. Thanks Michael.
Your argument about the duplicate events is a reasonable one. I considered it, but thought that either people weren't using zope.app.wsgi.paste or didn't care about the event, given how long the problem has existed. I could reverese the change with a 3.9.3zc3 release. That would force me into trying to figure out the breakages introduced by "later" releases, or fork or monkey patch zope.app.wsgi. I'd rather not. :) Given that an unpinned fetch of zope.app.wsgi won't get 3.9.3zc2 I'm thinking that my small foul is unlikely to do harm.
I'll also release this change as zope.app.wsgi 3.15.0, or do you think this should be 4.0.0?
Jim
-- Jim Fulton http://www.linkedin.com/in/jimfulton
On Thu, Jan 19, 2012 at 06:30:35AM -0500, Jim Fulton wrote:
On Wed, Jan 18, 2012 at 6:50 PM, Marius Gedminas marius@gedmin.as wrote:
(We are pinning all our dependency versions with buildout-versions, so our app won't break if you rele--oh, I see you already released 3.9.3zc2 with the change.)
Yup, yup.
...
Your argument about the duplicate events is a reasonable one. I considered it, but thought that either people weren't using zope.app.wsgi.paste or didn't care about the event, given how long the problem has existed. I could reverese the change with a 3.9.3zc3 release.
There's no need to do that on my behalf.
I doubt anyone will accidentally upgrade to 3.9.3zc2 given that 3.14.0 is currently the latest on PyPI.
That would force me into trying to figure out the breakages introduced by "later" releases, or fork or monkey patch zope.app.wsgi. I'd rather not. :) Given that an unpinned fetch of zope.app.wsgi won't get 3.9.3zc2 I'm thinking that my small foul is unlikely to do harm.
And the irregularity of the 'zc' version number is sufficient to pique the curiosity of people and have them look in the changelog, I think.
I'll also release this change as zope.app.wsgi 3.15.0, or do you think this should be 4.0.0?
3.15.0 sounds about right to me.
Marius Gedminas
On Thu, Jan 19, 2012 at 8:56 AM, Marius Gedminas marius@gedmin.as wrote:
On Thu, Jan 19, 2012 at 06:30:35AM -0500, Jim Fulton wrote:
..
I'll also release this change as zope.app.wsgi 3.15.0, or do you think this should be 4.0.0?
3.15.0 sounds about right to me.
Done.
Jim
-- Jim Fulton http://www.linkedin.com/in/jimfulton
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?
Thanks J-C
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?
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.
Should I rather create a ticket on launchpad? Or otherwise, should I just go ahead and merge the changes to trunk?
Thanks JC
Hiya,
Am 24.01.2012, 12:35 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
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've only glanced cursorily at the source but I'm not sure that a merge is okay yet. At least not all of the methods have doc strings and one of them seems to have a doctest. It would be nice to expand the README here. getTerm is a copy of the SimpleVocabulary method. Using the @classmethod decorator is fine but inconsistent with other vocabulary classes. Using the decorator is more readable so I'd suggest changing all relevant methods to do that. Off the top of my head I don't know which Python versions support classmethods. That should be checked in case zope.schema is in a ZTK that is running on Python 2.4
I would just like someone to sign it off. Should I rather create a ticket on launchpad? Or otherwise, should I just go ahead and merge the changes to trunk?
I would suggest going via launchpad if only because it is a better paper trail. At the moment I'm still not quite sure whether this is required in zope.schema. Do widgets only exist for z3c.form?
Charlie
On 01/24/2012 03:06 PM, Charlie Clark wrote:
Off the top of my head I don't know which Python versions support classmethods.
Classmethods are fine in python2.4
Cheers
Hi Charlie
On Tue, 2012-01-24 at 15:06 +0100, Charlie Clark wrote:
Hiya,
Am 24.01.2012, 12:35 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
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've only glanced cursorily at the source but I'm not sure that a merge is okay yet.
Yes, Marius pointed that out as well.
At least not all of the methods have doc strings and one of them seems to have a doctest.
I've clarified some of the docstrings and added the missing one.
None have doctests, perhaps you are referring to fromDict, which gives an example dict to show the required structure.
I guess that could easily be turned into a doctest, I'll look into it.
It would be nice to expand the README here.
I don't see anything about vocabs there at all, but I'm willing to add some tests.
getTerm is a copy of the SimpleVocabulary method. Using the @classmethod decorator is fine but inconsistent with other vocabulary classes. Using the decorator is more readable so I'd suggest changing all relevant methods to do that.
Ok, I changed all the other methods to also use the decorator.
Off the top of my head I don't know which Python versions support classmethods. That should be checked in case zope.schema is in a ZTK that is running on Python 2.4
Doesn't seem to be a problem for Python 2.4: http://www.python.org/dev/peps/pep-0318/#current-syntax
I would just like someone to sign it off. Should I rather create a ticket on launchpad? Or otherwise, should I just go ahead and merge the changes to trunk?
I would suggest going via launchpad if only because it is a better paper trail.
Ok.
At the moment I'm still not quite sure whether this is required in zope.schema.
I think that a zope3 style TreeVocabulary is indeed needed. We use them quite a bit in Plone (currently only for Archetypes). If not in zope.schema, then in a separate egg?
Do widgets only exist for z3c.form?
Well, there wasn't such a widget for z3c.form. The dynatree widget was for Archetypes. I (and Johan Beyers) ported it to z3c.form.
Thanks for taking the time. JC
Hiya,
Am 24.01.2012, 18:48 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
I've clarified some of the docstrings and added the missing one. None have doctests, perhaps you are referring to fromDict, which gives an example dict to show the required structure. I guess that could easily be turned into a doctest, I'll look into it.
No need to add doctests. It was more a comment on the docstring of one method in comparison with the others.
It would be nice to expand the README here.
I don't see anything about vocabs there at all, but I'm willing to add some tests.
er, just because the existing documentation is pants doesn't mean it can't be improved upon! ;-)
I'm still not sure about having TreeVocabulary in zope.schema if it is only going to be used with, shudder, Archetypes. On the one hand schema are theoretically dissociated from any form library and zope.form is already incomplete, on the other we try and avoid application-specific requirements in the libraries. All the more important to expand the documentation so that other libraries can benefit from the plumbing.
Charlie
Hi Charlie
On Wed, 2012-01-25 at 10:37 +0100, Charlie Clark wrote:
Hiya,
Am 24.01.2012, 18:48 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
I've clarified some of the docstrings and added the missing one. None have doctests, perhaps you are referring to fromDict, which gives an example dict to show the required structure. I guess that could easily be turned into a doctest, I'll look into it.
No need to add doctests. It was more a comment on the docstring of one method in comparison with the others.
It would be nice to expand the README here.
I don't see anything about vocabs there at all, but I'm willing to add some tests.
er, just because the existing documentation is pants doesn't mean it can't be improved upon! ;-)
I'm still not sure about having TreeVocabulary in zope.schema if it is only going to be used with, shudder, Archetypes.
It's *not* for use with Archetypes. :) That's what for example Products.ATVocabularyManager is for.
I just mentioned that this is a fairly common use-case in Plone, but up to now only with Archetypes, because a zope3-component type TreeVocabulary didn't exist yet. That's why I wrote this one.
On the one hand schema are theoretically dissociated from any form library and zope.form is already incomplete, on the other we try and avoid application-specific requirements in the libraries.
Sure. Like SimpleVocabulary, the Treevocabulary is not dependent on any form library.
In my case, I use it with z3c.form, but it could also be used with for example zope.formlib or any other form library that couples with zope.schema.
All the more important to expand the documentation so that other libraries can benefit from the plumbing.
I'll see what I can do.
JC
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:
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.
vocabulary.py, line 150:
"gne or more interfaces may also be provided so that alternate"
s/gne/One/
test_vocabulary.py, line 223:
self.assertTrue(dict, type(v._terms))
s/assertTrue/assertEqual/
test_vocabulary.py, line 249:
""" len returns the number of all nodes in die dict
s/die/the/
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 ITreeVocabulary.
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
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
On Tue, Jan 24, 2012 at 07:34:03PM +0200, Jan-Carel Brand wrote:
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.
Ok. But why Persistent? None of the other vocabularies are persistent...
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.
*nod*
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.
So is it PersistentMapping or PersistentDict then? ;)
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.
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.
Regards, Marius Gedminas
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:
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.
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.
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.
*nod*
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.
So is it PersistentMapping or PersistentDict then? ;)
It was first the one, and then the other :)
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
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.
JC
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
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".
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.
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.
- 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 )
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 )
Hiya,
Am 26.01.2012, 15:02 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
Ok, Charlie also expressed his reservations. I'll put it in a different package then.
Hang on a minute! While I'm not 100 % convinced of the need in the core I think a separate package just for TreeVocabulary would be splitting hairs. If z3c.form can use it then I think that is justification enough.
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.
I agree. Despite my previous remark about class methods, I don't think we need to worry much about Python 2.4 and 2.5 and ordered dictionaries are just so damn useful that they've been added to the standard library.
Back to bike-shedding. As I was intrigued by the whole thing I've spent some time looking at the code. I'm not too happy on the use of nested functions as I find they obscure code, particularly when used recursively. I think that "createTree" and "recurse" should be written as separately testable generators. I also don't see a need for createTerm in this particular class and the subsequent awkward call from createTree. As it stands it is copy & paste both in method and test. If you must have it with the same implementation
createTree = SimpleVocabulary.createTree
does the job just fine but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
As I said this is bike-shedding but I think our source code should be written with a view to being read and probably copied verbatim. With that in mind I prefer readability and testability over integration. In the end it tends to make things easier to use. The exceptions where refactoring to produce slightly uglier code but with significant performance hopefully prove the rule.
Charlie
On Thu, 2012-01-26 at 15:42 +0100, Charlie Clark wrote:
Hiya,
Am 26.01.2012, 15:02 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
Ok, Charlie also expressed his reservations. I'll put it in a different package then.
Hang on a minute! While I'm not 100 % convinced of the need in the core I think a separate package just for TreeVocabulary would be splitting hairs. If z3c.form can use it then I think that is justification enough.
Justification enough to put it in zope.schema?
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.
I agree. Despite my previous remark about class methods, I don't think we need to worry much about Python 2.4 and 2.5 and ordered dictionaries are just so damn useful that they've been added to the standard library.
From the zope.schema 4.0.0 release notes:
"Port to Python 3. This adds a dependency on six and removes support for Python 2.5"
So if we want to use OrderedDict (which is from Python >= 2.7), we just need to bridge the Python 2.6 version with the ordereddict package:
http://pypi.python.org/pypi/ordereddict/1.1
This would introduce a new dependency for zope.schema on Python 2.6, I don't know if that's acceptable or not.
In setup.py one could specify the extra dependency only for Python < 2.7:
import sys
REQUIRES = ['setuptools', 'zope.interface >= 3.6.0', 'zope.event', 'six', ]
if sys.version_info < (2 , 7): REQUIRES += ['ordereddict'],
setup( # [...] install_requires=REQUIRES, # [...] )
On Thu, 2012-01-26 at 15:42 +0100, Charlie Clark wrote:
Back to bike-shedding. As I was intrigued by the whole thing I've spent some time looking at the code. I'm not too happy on the use of nested functions as I find they obscure code, particularly when used recursively. I think that "createTree" and "recurse" should be written as separately testable generators.
Ok, I've refactored the nested methods out into class or instance methods.
I however don't see how one could use a generator for the recursive methods that return dictionaries.
With regards to the "recurse" method (now called _getPathToTreeNode), I don't see how one could use a generator in a more efficient manner than the normal recursion that's being used currently.
I played around with it and the best I could come up with is this:
def generator(_dict, value, path=[]): if value in _dict.keys(): yield path+[value]
for key in _dict.keys(): for path in recurse(_dict[key], value, path+[key]): if value in path: yield path
You still have to recurse through the different branches until you find the node that you are looking for and you still need to store the path in a list. So what would be the added value?
What's more, the generator returns a list within a list, like so: [['Regions', 'Germany', 'Bavaria']], which I find clunky.
I also don't see a need for createTerm in this particular class and the subsequent awkward call from createTree. As it stands it is copy & paste both in method and test.
The reason for the "createTerm" method in the SimpleVocabulary is to allow people to override it to provide support for other term types.
This also applies to the TreeVocabulary.
If you must have it with the same implementation
createTree = SimpleVocabulary.createTree
does the job just fine
I guess you mean "createTerm" ?
I'm not convinced that this is better.
This creates a dependency on a method from another class that's not being subclassed.
What if createTerm is changed in the SimpleVocabulary in such a way that doesn't take the TreeVocabulary into account?
but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
See above. "createTerm" is there to let developers override it and provide their own term objects.
As I said this is bike-shedding but I think our source code should be written with a view to being read and probably copied verbatim. With that in mind I prefer readability and testability over integration.
So why then cannot I copy "createTerm" from SimpleVocabulary? :D
In the end it tends to make things easier to use. The exceptions where refactoring to produce slightly uglier code but with significant performance hopefully prove the rule.
Well, your suggestions concerning the nested methods had me thinking and in the end resulted in significant refactoring. I think it was worth it though, so thanks.
J-C
Hiya,
Am 27.01.2012, 12:56 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
Hang on a minute! While I'm not 100 % convinced of the need in the core I think a separate package just for TreeVocabulary would be splitting hairs. If z3c.form can use it then I think that is justification enough.
Justification enough to put it in zope.schema?
Yes.
From the zope.schema 4.0.0 release notes: "Port to Python 3. This adds a dependency on six and removes support for Python 2.5"
Shouldn't that also mean a preference of "for key in _dict" over "for key in _dict.keys()" ?
Though you might prefer .items() in _getPathToTreeNode()
i.e def _getPathToTreeNode(self, tree, node)
path = [] for parent, child in tree.items(): if node == parent.value: return [node] path = self._getPathToTreeNode(child, node) if path: path.insert(0, parent.value) break return path
So if we want to use OrderedDict (which is from Python >= 2.7), we just need to bridge the Python 2.6 version with the ordereddict package: http://pypi.python.org/pypi/ordereddict/1.1 This would introduce a new dependency for zope.schema on Python 2.6, I don't know if that's acceptable or not.
I think it's perfectly justified in this case and similar to what has happened with other libraries like ElementTree in the past that have made life easier and subsequently been adopted by the standard library.
In setup.py one could specify the extra dependency only for Python < 2.7:
import sys
REQUIRES = ['setuptools', 'zope.interface >= 3.6.0', 'zope.event', 'six', ]
if sys.version_info < (2 , 7): REQUIRES += ['ordereddict'],
setup( # [...] install_requires=REQUIRES, # [...]
Yep.
Back to bike-shedding. As I was intrigued by the whole thing I've spent some time looking at the code. I'm not too happy on the use of nested functions as I find they obscure code, particularly when used recursively. I think that "createTree" and "recurse" should be written as separately testable generators.
Ok, I've refactored the nested methods out into class or instance methods. I however don't see how one could use a generator for the recursive methods that return dictionaries. With regards to the "recurse" method (now called _getPathToTreeNode), I don't see how one could use a generator in a more efficient manner than the normal recursion that's being used currently. I played around with it and the best I could come up with is this: def generator(_dict, value, path=[]): if value in _dict.keys(): yield path+[value] for key in _dict.keys(): for path in recurse(_dict[key], value, path+[key]): if value in path: yield path You still have to recurse through the different branches until you find the node that you are looking for and you still need to store the path in a list. So what would be the added value? What's more, the generator returns a list within a list, like so: [['Regions', 'Germany', 'Bavaria']], which I find clunky.
You're probably right. I think I was getting ahead of myself wondering about possible issues with deeply nested vocabularies. Any real improvement would probably involve a node index. I notice that the examples do not allow for departments in regions to have the same name as the region (common enough in some countries) so you could simply add the index keyed by node value with the path as the value when the class is created. This should be possible by calling _getPathToTreeNode during one of the passes through _flattenTree. getTermPath would then just need to do a lookup on this.
I also don't see a need for createTerm in this particular class and the subsequent awkward call from createTree. As it stands it is copy & paste both in method and test.
The reason for the "createTerm" method in the SimpleVocabulary is to allow people to override it to provide support for other term types. This also applies to the TreeVocabulary.
I think this is an example of excessive pluggability. createTerm isn't in an interface and I've never come across the need to overwrite it. I would just drop it from the implementation. Problem solved. fromDict() seems to be the only class method required.
If you must have it with the same implementation
e
createTree = SimpleVocabulary.createTree
does the job just fine
I guess you mean "createTerm" ? I'm not convinced that this is better. This creates a dependency on a method from another class that's not being subclassed.
What if createTerm is changed in the SimpleVocabulary in such a way that doesn't take the TreeVocabulary into account?
That would be the case with any subclass. In terms of maintenance it would be easier to spot in case the changed behaviour was desirable.
but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
See above. "createTerm" is there to let developers override it and provide their own term objects.
Do you have a concrete use case for this? Remember that createTerm is a convenience method only. Frankly, I don't see the need for it in what is a fairly specialised class.
As I said this is bike-shedding but I think our source code should be written with a view to being read and probably copied verbatim. With that in mind I prefer readability and testability over integration.
So why then cannot I copy "createTerm" from SimpleVocabulary?
For exactly that reason: just because someone writing application might copy & paste your code is should be reason enough to make the code as clean as possible and that does mean DRY.
In the end it tends to make things easier to use. The exceptions where refactoring to produce slightly uglier code but with significant performance hopefully prove the rule.
Well, your suggestions concerning the nested methods had me thinking and in the end resulted in significant refactoring. I think it was worth it though, so thanks.
I've enclosed a diff with proposed changes.
Charlie
On Sun, 2012-01-29 at 16:17 +0100, Charlie Clark wrote:
<snip>
From the zope.schema 4.0.0 release notes: "Port to Python 3. This adds a dependency on six and removes support for Python 2.5"
Shouldn't that also mean a preference of "for key in _dict" over "for key in _dict.keys()" ?
Though you might prefer .items() in _getPathToTreeNode()
i.e def _getPathToTreeNode(self, tree, node)
path = [] for parent, child in tree.items(): if node == parent.value: return [node] path = self._getPathToTreeNode(child, node) if path: path.insert(0, parent.value) break return path
Thanks, I've taken this code from the diff file you gave.
So if we want to use OrderedDict (which is from Python >= 2.7), we just need to bridge the Python 2.6 version with the ordereddict package: http://pypi.python.org/pypi/ordereddict/1.1 This would introduce a new dependency for zope.schema on Python 2.6, I don't know if that's acceptable or not.
I think it's perfectly justified in this case and similar to what has happened with other libraries like ElementTree in the past that have made life easier and subsequently been adopted by the standard library.
Marius and Souheil, what do you think about this? I think it probably still makes sense to have a dict_factory as Souheil suggested, but then we make the default type OrderedDict and not dict.
Any objections?
In setup.py one could specify the extra dependency only for Python < 2.7:
import sys
REQUIRES = ['setuptools', 'zope.interface >= 3.6.0', 'zope.event', 'six', ]
if sys.version_info < (2 , 7): REQUIRES += ['ordereddict'],
setup( # [...] install_requires=REQUIRES, # [...]
Yep.
<snip>
I however don't see how one could use a generator for the recursive methods that return dictionaries. With regards to the "recurse" method (now called _getPathToTreeNode), I don't see how one could use a generator in a more efficient manner than the normal recursion that's being used currently. I played around with it and the best I could come up with is this: def generator(_dict, value, path=[]): if value in _dict.keys(): yield path+[value] for key in _dict.keys(): for path in recurse(_dict[key], value, path+[key]): if value in path: yield path You still have to recurse through the different branches until you find the node that you are looking for and you still need to store the path in a list. So what would be the added value? What's more, the generator returns a list within a list, like so: [['Regions', 'Germany', 'Bavaria']], which I find clunky.
You're probably right. I think I was getting ahead of myself wondering about possible issues with deeply nested vocabularies.
Any real improvement would probably involve a node index. I notice that the examples do not allow for departments in regions to have the same name as the region (common enough in some countries) so you could simply add the index keyed by node value with the path as the value when the class is created.
Yes, the values must be unique, because we do value lookups.
The "title" attr doesn't have to be unique though. You could have a "Forestry" department for two different regions. The title will be the same, but the value and token attrs can't.
This should be possible by calling _getPathToTreeNode during one of the passes through _flattenTree. getTermPath would then just need to do a lookup on this.
I don't like the way the path_node gets implicitly populated during a call to _flattenTree.
I'd rather have a separate method that calculates the path and then explicitly assign it to self.path_node.
In any case, there is now a node_index in the code :)
<snip>
but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
See above. "createTerm" is there to let developers override it and provide their own term objects.
Do you have a concrete use case for this?
Not really, but that doesn't mean it doesn't exist.
Remember that createTerm is a convenience method only. Frankly, I don't see the need for it in what is a fairly specialised class.
I like consistency and symmetry, so if SimpleVocabulary has it, as an add-on developer I'd expect for TreeVocabulary to also have it.
I don't however feel very strongly about it though, and I wanna get this done, so I removed it.
J-C
Hiya,
Am 30.01.2012, 14:33 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
... lots cut ...
Yes, the values must be unique, because we do value lookups. The "title" attr doesn't have to be unique though. You could have a "Forestry" department for two different regions. The title will be the same, but the value and token attrs can't.
I think the tests should be extended to show that this also includes nesting because this is non-obvious. ie. I believe that the region "Izmir" is within the state of "Izimir" in Turkey.
This should be possible by calling _getPathToTreeNode during one of the passes through _flattenTree. getTermPath would then just need to do a lookup on this.
I don't like the way the path_node gets implicitly populated during a call to _flattenTree.
hm, okay. Personally, I think you should be able to populate your dictionaries with only a single pass through the terms. However, as this only needs to happen when the application starts we don't need to worry too much about the performance.
I'd rather have a separate method that calculates the path and then explicitly assign it to self.path_node. In any case, there is now a node_index in the code
<snip>
but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
See above. "createTerm" is there to let developers override it and provide their own term objects.
Do you have a concrete use case for this?
Not really, but that doesn't mean it doesn't exist.
Then someone will speak up for it if they need it or do their own subclassing/composition as required. Otherwise it's just food for warts.
Remember that createTerm is a convenience method only. Frankly, I don't see the need for it in what is a fairly specialised class.
I like consistency and symmetry, so if SimpleVocabulary has it, as an add-on developer I'd expect for TreeVocabulary to also have it. I don't however feel very strongly about it though, and I wanna get this done, so I removed it.
Well, we could always think about removing it from SimpleVocabulary: it's not in the interface so no subclass actually has the right to depend on it. ;-)
Charlie
On Mon, 2012-01-30 at 15:42 +0100, Charlie Clark wrote:
Am 30.01.2012, 14:33 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
... lots cut ...
Yes, the values must be unique, because we do value lookups. The "title" attr doesn't have to be unique though. You could have a "Forestry" department for two different regions. The title will be the same, but the value and token attrs can't.
I think the tests should be extended to show that this also includes nesting because this is non-obvious. ie. I believe that the region "Izmir" is within the state of "Izimir" in Turkey.
Ok, I've added the method *test_nonunique_values_and_tokens* to the tests.
This should be possible by calling _getPathToTreeNode during one of the passes through _flattenTree. getTermPath would then just need to do a lookup on this.
I don't like the way the path_node gets implicitly populated during a call to _flattenTree.
hm, okay. Personally, I think you should be able to populate your dictionaries with only a single pass through the terms. However, as this only needs to happen when the application starts we don't need to worry too much about the performance.
I think you're right, so I've refactored _flattenTree and _getPathIndex into a single method _populateIndexes that populates all three indexes with one tree-traversal.
I'd rather have a separate method that calculates the path and then explicitly assign it to self.path_node. In any case, there is now a node_index in the code
<snip>
but I don't see the advantage of cls.createTerm(*args) over SimpleTerm(*args)
See above. "createTerm" is there to let developers override it and provide their own term objects.
Do you have a concrete use case for this?
Not really, but that doesn't mean it doesn't exist.
Then someone will speak up for it if they need it or do their own subclassing/composition as required. Otherwise it's just food for warts.
Remember that createTerm is a convenience method only. Frankly, I don't see the need for it in what is a fairly specialised class.
I like consistency and symmetry, so if SimpleVocabulary has it, as an add-on developer I'd expect for TreeVocabulary to also have it. I don't however feel very strongly about it though, and I wanna get this done, so I removed it.
Well, we could always think about removing it from SimpleVocabulary: it's not in the interface so no subclass actually has the right to depend on it. ;-)
Looking at the documentation in fields.txt I see the following:
A vocabulary must minimally be able to determine whether it contains a value, to create a term object for a value, and to return a query interface (or None) to find items in itself.
So it looks like someone thinks that any vocabulary must be able to create a term object. Thoughts on that?
I think I'm now finished and have implemented everybody's suggestions and improvements.
The TreeVocabulary now has a terms_factory attribute which is an OrderedDict by default and which can be overridden in a subclass to an alternative datastructure. It also implements IEnumerableMapping.
All the TreeVocabulary methods are covered by tests and I tested with Python 2.6 and Python 2.7.
If there aren't any more objections, I'd like to now merge with trunk.
Regards J-C
Hiya,
... lots cut ...
I like the _populateIndexes() method. Having a single pass without a signifying parameter makes it easier to understand.
Am 06.02.2012, 10:12 Uhr, schrieb Jan-Carel Brand lists@opkode.com:
A vocabulary must minimally be able to determine whether it contains a value, to create a term object for a value, and to return a query interface (or None) to find items in itself.
So it looks like someone thinks that any vocabulary must be able to create a term object. Thoughts on that?
Yes: I think it's much more important to define what kind of terms are expected. Apart from local utilities I've never come across the need to add terms individually and even then term generation is outside the scope of the vocabulary as you already have a term factory. Validating terms seems more important so I guess and __setitem__() method which imposes the same checks as you run in _populateIndexes() might be worthwhile to stop the index being fouled up by application code.
I think I'm now finished and have implemented everybody's suggestions and improvements.
The TreeVocabulary now has a terms_factory attribute which is an OrderedDict by default and which can be overridden in a subclass to an alternative datastructure. It also implements IEnumerableMapping. All the TreeVocabulary methods are covered by tests and I tested with Python 2.6 and Python 2.7. If there aren't any more objections, I'd like to now merge with trunk.
I think it's okay to go ahead and merge.
Charlie
Snip snip
Marius and Souheil, what do you think about this? I think it probably still makes sense to have a dict_factory as Souheil suggested, but then we make the default type OrderedDict and not dict.
This sounds perfectly reasonnable.
Any objections?
Not really
On Thu, Jan 26, 2012 at 04:02:54PM +0200, Jan-Carel Brand wrote:
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?
My gut feeling says "z3c".
OTOH I don't think a single class warrants a whole separate PyPI package. Especially if it's going to be used with z3c.form (hint, hint ;) or that collective.forgotthenamealready you mentioned upthread.
I'm guessing zope.vocabulary, or rather zope.treevocabulary?
(Or z3c.treevocabulary)
But I'd rather see this in zope.schema.
Incidentally, since this would be a new feature, it would need a minor version bump in setup.py (4.0.2dev -> 4.1dev).
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?
I think he meant something like
class TreeVocabulary(object):
# you can subclass and use OrderedDictionary instead dict_factory = dict
This would mean that you can't subclass dict and would instead have to delegate __getitem__, __iter__, __len__, keys, values, items and all the rest by hand.
I'm actually feeling a bit guilty about raising this question. As I said, I don't have a use-case for ordered TreeVocabularies myself. (Or unordered ones either, for that matter ;) The only reason for hashing this out now is to avoid painful API changes if we ever decide that we need those. Feel free to cry YAGNI at any time. It's already hard enough to contribute to zope3-or-is-it-bluebream-or-is-it-ztk-aaaugh as it is, without us adding extra barriers in place.
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
Oh, neat. I've used http://pypi.python.org/pypi/odict in the past, but it doesn't have the blessing of stdlib'ness.
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.
Thanks!
Marius Gedminas
Yes, Marius got exactly what I meant, this is how I usually make the not-so-pluggable stuff pluggable :) About the fact to release it out of zope.schema, it's also to be able to make it live a bit and prove itself. Then, including it and changing the API can be easily justified. I'm not afraid of the changes in zope.schema. z3c.form is not the only form option, I'll probably use it in dolmen.forms that came from zeam.form. So, whatever you decide, we'll try to make it easy for you to contribute. It tends to get tooooo complicated to do anything in zope, lately :)
2012/1/26 Marius Gedminas marius@gedmin.as:
On Thu, Jan 26, 2012 at 04:02:54PM +0200, Jan-Carel Brand wrote:
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?
My gut feeling says "z3c".
OTOH I don't think a single class warrants a whole separate PyPI package. Especially if it's going to be used with z3c.form (hint, hint ;) or that collective.forgotthenamealready you mentioned upthread.
I'm guessing zope.vocabulary, or rather zope.treevocabulary?
(Or z3c.treevocabulary)
But I'd rather see this in zope.schema.
Incidentally, since this would be a new feature, it would need a minor version bump in setup.py (4.0.2dev -> 4.1dev).
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?
I think he meant something like
class TreeVocabulary(object):
# you can subclass and use OrderedDictionary instead dict_factory = dict
This would mean that you can't subclass dict and would instead have to delegate __getitem__, __iter__, __len__, keys, values, items and all the rest by hand.
I'm actually feeling a bit guilty about raising this question. As I said, I don't have a use-case for ordered TreeVocabularies myself. (Or unordered ones either, for that matter ;) The only reason for hashing this out now is to avoid painful API changes if we ever decide that we need those. Feel free to cry YAGNI at any time. It's already hard enough to contribute to zope3-or-is-it-bluebream-or-is-it-ztk-aaaugh as it is, without us adding extra barriers in place.
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
Oh, neat. I've used http://pypi.python.org/pypi/odict in the past, but it doesn't have the blessing of stdlib'ness.
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.
Thanks!
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 )
On Fri, 2012-01-27 at 11:50 +0100, Souheil CHELFOUH wrote:
Yes, Marius got exactly what I meant, this is how I usually make the not-so-pluggable stuff pluggable :) About the fact to release it out of zope.schema, it's also to be able to make it live a bit and prove itself. Then, including it and changing the API can be easily justified. I'm not afraid of the changes in zope.schema. z3c.form is not the only form option, I'll probably use it in dolmen.forms that came from zeam.form. So, whatever you decide, we'll try to make it easy for you to contribute. It tends to get tooooo complicated to do anything in zope, lately :)
Ok, I've implemented the idea of the dict_factory, making sure to implement all the methods required by IEnumerableMapping and IReadMapping. All tests pass and it works in my specific end use-case.
However, since we can use the OrderedDict type in Python < 2.7 by means of the ordereddict package, what do you think of making that the default dict type and adding an 'ordereddict' dependency for older Python versions? (Please see Charlie and my discussion about it.)
If we still use the concept of a dict_factory, it wouldn't make sense to implement the OrderedDict specific methods like "popItem", but the "keys" method would be ordered, which I think would be enough.
J-C
-
Charlie Clark -
Georges Racinet -
Jan-Carel Brand -
Jim Fulton -
Marius Gedminas -
Michael Howitz -
Souheil CHELFOUH