[PATCH 3 of 3 eac] Modified decorator for child appending
denis.laxalde at logilab.fr
Thu Jul 4 16:15:41 CEST 2019
Guillaume Vandevelde a écrit :
> # HG changeset patch
> # User Guillaume Vandevelde <gvandevelde at logilab.fr>
> # Date 1560762819 -7200
> # Mon Jun 17 11:13:39 2019 +0200
> # Node ID 3e2d6229319f256964f46072208034e9089dbedd
> # Parent 7049863207c0a5bd19efa5d4291d59229243473b
> # Available At http://hg.logilab.org/review/cubes/eac
> # hg pull http://hg.logilab.org/review/cubes/eac -r 3e2d6229319f
Some remarks about the commit message:
> Modified decorator for child appending
First line should be in the imperative and almost self-explanatory.
> Modified the `add_citation_for` decorator so it can handle any child appending.
> Modified the History child creation and concatenation so it can use this system.
Then, the rest of the message should consists of sentences.
> I don't like that the `builders` dictionnary in the `add_child_for` decorator is loaded every time a decorated function is called, but I can't get it back of the `wrapper` function because of the dependency of 'self' in the build functions.
Line too long, needs wrapping.
About the actual question in this sentence, I see two possibilities:
* use build_child = getattr(self, 'build_' + relation)
* make add_child_for accept a build method name as argument and add
concrete version of it as:
add_citations_for = partial(add_child_for, relation='has_citation', method='build_citation')
add_events_for = partial(add_child_for, relation='has_event', method='build_event')
(I tend to prefer the second option.)
Then, if I understand correctly, this patch would be better before the
previous one, so that the latter would be implement in its final form
directly. So would it possible to swap them? This would also make this
patch simpler to review (and independent of the previous one).
Finally, I don't understand why tests are changed by this refactoring.
SO if this is justified, this needs to be explained in the commit
> Differential Revision: https://phab.logilab.fr/D3467
Can you remove these lines?
More information about the saem-devel