Comment 35 for bug 525808

Lionel is of course right here. However in the few functions that do amend
context, the vast majority already copy and pass the copied dict, certainly
I don't recall ever seeing it in main addons branch where it wasn't. But
most function calls do nothing with the context (in which case the entire
recommended code block is redundant) and the very few that do are for the
most part just looking up values, which makes the second half of the code
really unnecessary. Why add extra processing instructions if they are
simply not required. Also the recommendation code cause more issues.
 Often the modified context is to add a variable like a date, period,
location whatever to another call. Generally where I see it it is always
done on a copy of the context, ctx, ctx2, ctx3 being the common
conventions. Given that differently modified contexts are foreseeably
passed to different functions and then require the original context in
future calls, I think leaving the context copying until it is required is
more readable.

The only exception I see to this regularly is in wizard code, which
possibly should be cleaned up, although possibly it is the special case.

I also fail to see a case where a caller would expect to have its context
modified, except maybe that of a helper function specifically designed to
amend the context of multiple callers to facilitate reuse, e.g.
_set_location_context(blah blah) in which case documenting it may be
unnecessary but even then for most cases you wouldn't do it that way.

But would be nice to have added to developer guidelines. The other one I
still see which really bugs me is the good old default context={} in
function defs.

2012/3/7 Numérigraphe <email address hidden>

> Le 06/03/2012 10:26, Niels Huylebroeck a écrit :
> > This depends on whether or not the called function wants to alter the
> behavior (...) of the calling function and thus seems optional and depends
> on the case.
> Ghostbusters don't cross the streams, though it depends on the case too.
> Usually in OpenERP the context is updated to adjust the behavior of
> subsequent function calls, not that of the caller.
> For example, there's no reason to mutate the context in an ORM method is
> there?
> Furthermore, there is no way a developer can know the impact of changing
> a key in the caller's context, so at first sight I'd consider it a mistake.
> Legitimate uses are possible but they would be rare, wouldn't they? They
> should be explicitly documented with a comment.
> > I'd just like to point out that the context is not passed with a
> "pointer" or anything like that(...)
> Sorry about the C-ish description. The important part was : it's mutable.
>
> Lionel.
>
> --
> You received this bug notification because you are subscribed to OpenERP
> Server.
> https://bugs.launchpad.net/bugs/525808
>
> Title:
> Default values should be immutable
>
> Status in OpenERP Addons (modules):
> Opinion
> Status in OpenERP Addons extra-trunk series:
> Opinion
> Status in OpenERP Addons trunk series:
> Opinion
> Status in OpenERP GTK Client:
> Opinion
> Status in OpenERP Server:
> Fix Released
> Status in OpenERP Server trunk series:
> Fix Released
>
> Bug description:
> Often in the code base we have:
> def foo(...., context={},....):
> ....
> As explained in comment #1, this should be changed to
> def foo(...., context=None,....):
> # you should add this test only if the context is actually used
> if context is None:
> context={}
> ....
>
> (This bug originally proposed the other way round hence the "nope" in
> comment #1.)
>
> (xmo) Complements of information:
>
> * The test should be `context is None` rather than `not context`: the
> caller could provide an empty dict argument {} and expect to have data in
> it after the call, if a test is done using `not context` an empty dict will
> match and the function will be creating a brand new context.
> * The test should *only ever* performed when the current function fetches
> data from the context. If you don't do anything with the context but
> forward it to other functions and methods (e.g. read() or create()), no
> need to test for anything.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/openobject-addons/+bug/525808/+subscriptions
>