[PATCH 1 of 2 compound v2] Propagate skiprtypes to subentities cw_skip_copy_for while cloning
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