[Zope-CMF] Re: GenericSetup improvements

[yuppie]

Hi Florent!


Florent Guillaume wrote:
> On 27 Dec 2005, at 10:51, yuppie wrote:
>> Florent Guillaume wrote:
>>> Also I'd like to change things like:
>>>     node = property(_exportNode, _importNode)
>>> into:
>>>     def __exportNode(self):
>>>         return self._exportNode()
>>>     def __importNode(self, *args)
>>>         self._importNode(*args)
>>>     node = property(__exportNode, __importNode)
>>> To allow proper subclass overriding and avoid the dead chicken of 
>>> having to redefine the node property each time.
> 
> Done.
> 
> But in my (complex) setup it revealed an unintended side effect. There 
> are actually 3 useful methods when doing export (also valid for import):
> 
> - export as simple node for its parent file (just the <object name="foo" 
> meta_type="bar"> part),
> - export as full node tree,
> - export as XML body using the "full node" code.

Correct.

> Before, the fact that you defined or not
>    node = property(_exportNode, _importNode)
> made the export "do the right thing", as
>  - _extractObjects calls exporter.node, thus calls the last node 
> property defined, whereas
>  - _exportBody calls self._exportNode(), thus inherits
> 
> This is *very* implicit and magic, and would benefit from some 
> refactoring...

Well. That's the way property() works. Your change adds magic because it 
makes property() inherit its methods.

> The current status of the code is that exports of more that one level 
> are currently not correct.
> It seems there's no unit test for it...

There are no real unit tests for it, but there are many unit tests for 
adapters using those base classes. These are at the same time 
integration tests for the base classes in GenericSetup.utils. Some 
CMFCore tests are failing after your change.

> I'll fix this in a few days unless someone is interested to do it before 
> then.

I don't think it's a good idea to make obviously incomplete checkins to 
the trunk. Why didn't you create a branch?

> I propose to have _exportSimpleNode for the basic version, _exportNode 
> for the full version, and have _extractObjects call _exportSimpleNode 
> and _exportBody call _exportNode. The node property would refer to the 
> full version.

_extractObjects sometimes needs the simple node (if the sub-object has 
its own XML file) and sometimes the full node (if the sub-object 
settings should be part of the parent's XML file).

Only the sub-object adapter knows which node _extractObjects needs. The 
node property should always return the correct node. _extractObjects 
should not call a private method of the sub-object adapter.

_exportBody always needs the full node, but it doesn't need to use a 
public method.

> Moreover, I propose to *deprecate* the .node and .body properties, I 
> don't see how they help readability and having properties do complex 
> work is really not intuitive to me. They're doing more harm than 
> anything else (anytime I read the code I have to scratch my head and 
> remember that it's not just an attribute assignment).

The setup handlers don't care about how the body or the node is stored 
inside the object. If this is internally a complex operation or a simple 
attribute assignment doesn't matter. From the setup handler point of 
view this is a simple read or write operation.

For read/write operations properties are intuitive to me. But if I'm the 
only one who thinks that way it's not too late to change INode and 
IBody. We are still in the alpha phase.


Cheers,

	Yuppie