Message39448
Logged In: YES
user_id=45365
Donovan, I love the functionality of your patch, but I would humbly request you make a couple of changes. Alternatively I'll make them, but that will delay the patch (as I have to find the time to do them).
First: please make it a context diff (cvs diff -c), as straight diffs are too error prone for moving targets. There are also mods I can't judge this way (such as why you moved the 'utxt' support in aepack.py to a different place. Or is this a whitespace mismatch?)
Second: you've diffed against the different version than against which you've patched. See gensuitemodule, for instance: it appears as if you've modified 1.22 but diffed against 1.21. Maybe you applied my 1.21->1.22 by hand without doing a cvs update? I think a cvs update (plus some manual work;-) should solve this.
Third: the passing of modules by name (to the decoding routines) seems error prone and not too elegant. Can't you pass the modules themselves in stead of their names? It would also save extra imports in the decoders.
Fourth: assigning to __bases__ seems like rather a big hack. Can't we generate the classes with a helper class, similarly to the event helper class in the __init__.py modules: FooApp/Foo_Suite.py would contain the class foo sketched above, and FooApp.__init__.py would contain
import othersuite.superfoo
import Foo_Suite.foo
class foo(Foo_Suite.foo, othersuite.superfoo):
pass
Fifth, and least important: you're manually iterating over the base classes for lookup. Couldn't we statically combine the _propdict and _elemdict's of the base classes during class declaration, so that at lookup time we'd need only a single dictionary lookup? The "class foo" body in __init__.py would then become something like
_propdict = aetools.flatten_dicts(
foosuite.foo._propdict,
othersuite.superfoo._propdict)
and similar for elemdict. |
|
Date |
User |
Action |
Args |
2007-08-23 15:11:59 | admin | link | issue538395 messages |
2007-08-23 15:11:59 | admin | create | |
|