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

Denis Laxalde denis.laxalde at logilab.fr
Tue Dec 19 10:48:22 CET 2017


Sylvain Thénault a écrit :
> 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.

Yet another issue with this modeling approach ;)

> 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.

Seems fine to me.



More information about the saem-devel mailing list