[PATCH 1 of 2 compound v2] Propagate skiprtypes to subentities cw_skip_copy_for while cloning

Sylvain Thénault sylvain.thenault at logilab.fr
Tue Dec 19 10:39:31 CET 2017



On 18/12/2017 09:01, Denis Laxalde wrote:
> Sylvain Thénault a écrit :
>> On 14/12/2017 09:16, Denis Laxalde wrote:
>>> Sylvain Thenault a écrit :
>>>> # HG changeset patch
>>>> # User Sylvain Thénault <sylvain.thenault at logilab.fr>
>>>> # Date 1513098203 -3600
>>>> #      Tue Dec 12 18:03:23 2017 +0100
>>>> # Node ID 0fc9b87651095ba0773fe2b623c05d525e564676
>>>> # Parent  00a664463fdbc9db154056f064e767dc54e3f2ab
>>>> Propagate skiprtypes to subentities cw_skip_copy_for while cloning
>>>>
>>>> We have to consider it at every levels of the container tree, not only for the
>>>> top-level entity.
>>>>
>>>> While doing so, it sounded good to move creation of the list to a cached
>>>> property to avoid redoing this for each cloned entity of the tree.
>>> I'm sold on this change... Long story short, I think things are getting
>>> a bit too complex to solve a use case which, if I understand correctly,
>>> could be solved differently with either a couple of declarations in
>>> client code or a different implementation of clone operations during
>>> graph traversal.
>>>
>>> By client code declarations, I mean (following new test schema added in
>>> this changeset), if I add:
>>>
>>> class Biography(AnyEntity):
>>>     __regid__ = 'Biography'
>>>     cw_skip_copy_for = [('see_also', 'subject')]
>>>
>>> doesn't it just solve the problem?
>> It does. My point is that as a client of the compound clone's feature, I
>> understand 'skiprtypes' as a centralized way to control what's to clone
>> / consider or not. If skiprtypes is only considered for relations of the
>> top-level container, it feels much less useful.
> So I've been thinking a bit about this and (now) agree that this is a
> legitimate feature request.
>
> I'm still not enthusiastic about how this is made possible in this
> patch. In particular, the live modification of "cw_skip_copy_for"
> attribute is a bit messy. In fact, I now realize that the pattern was
> already there for the top-level entity, so... I don't know what to
> suggest as an alternative. Not a big deal anyways, just mentioning...

I agree this is at best cumbersome, but I've not better proposition than
reimplementing copy_relations.


>
> Finally, about the "original vs self" issue in "copy_entity", I think
> that if "copy_entity" is to be kept as a method, then one improvement
> could be to have an extra "adaptation" of entities to be copied:
>
>   clones[child] = child.cw_adapt_to('IClonable').copy_entity(**kwargs)
>
> What do you think?

The problem with this proposition is that it implies every entity to be
IClonable, while currently only top-level entities are.
Regarding the SEDA cubes, this implies a huge number of entity types to
be IClonable, probably even enough to have a performance penalty when
evaluating the selector.

I would either keep it as is or just avoid turning the former function
into a method, by adding extra skip_rtypes as argument. That may be the
better compromise.


-- 
Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42)
Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations
Développement logiciel sur mesure:       http://www.logilab.fr/services
CubicWeb, the semantic web framework:    http://www.cubicweb.org



More information about the saem-devel mailing list