On Thu, 2012-01-26 at 15:42 +0100, Charlie Clark wrote:
> Hiya,
> 
> Am 26.01.2012, 15:02 Uhr, schrieb Jan-Carel Brand <li...@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

_______________________________________________
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 )

Reply via email to