[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 11:07:49 CET 2017


v3 sent


On 19/12/2017 10:48, Denis Laxalde wrote:
> 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.
>

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