Code review comment for lp:~therp-nl/banking-addons/ba70-lp1231174-default_method_should_not_raise_at_module_installation_time

Revision history for this message
Stéphane Bidoul (Acsone) (sbi) wrote :

Hi Pedro, all,

Irrespective of the performance aspects, it's more than a matter of taste
IMHO. If a method receives a dict in the context, even an empty one, the
same dict must be used, otherwise changes to the context would not be
propagated to the caller.

Communicating to the caller through the context may be hackish, but it's
the behaviour we expect when changing the context.

So context = context or {} would not be 100% correct.
Correct, albeit less readable, one liners would be
context = context if context is not None else {}
context = {} if context is None else context

My 2 cents.

-sbi

On Fri, Sep 27, 2013 at 1:59 AM, Pedro Manuel Baeza
<email address hidden>wrote:

> Hi, Ronald, thank you very much for your deep exploration on this topic. I
> see no clear winner then. I still preferred the one line option, and avoid
> hacks like ones mentioned by Stefan, but it's a matter of taste ;)
>
> Regards.
> --
>
> https://code.launchpad.net/~therp-nl/banking-addons/ba70-lp1231174-default_method_should_not_raise_at_module_installation_time/+merge/187677
> You are subscribed to branch lp:banking-addons.
>

« Back to merge proposal