openerp-expert-framework team mailing list archive

["Olivier Dony \(OpenERP\)" <525808@xxxxxxxxxxxxxxxxxx>]

On 03/06/2012 12:12 PM, Numérigraphe wrote:
> 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.

I tend to agree with Niels on this. Contrary to the mutable default parameter
which is an obscure Python caveat, developers cannot ignore when they are
mutating the request context.

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

There are many subtle cases where changing the context is needed, sometimes for
callees only, but sometimes also for the entire request.
For example in the ORM the copy_data() method needs both:
- it needs to read a different translation of a column by setting
context['lang'] (for callees only)
- it needs to maintain something like a context['visited_records'] set, in
order to detect cycles in an object graph (for the entire request)


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

The impact is usually pretty obvious. If you're altering a known context value
such as 'lang' or 'tz' you obviously know what you're doing, and that it should
be done in a copy of the context in most cases (but not always).
If you're simply setting a custom context key that your method and its callees
agree upon, then you should properly namespace it in order to avoid name
conflicts with someone else's custom key. And it does not matter that much if
you do it in a copy of the context or not.

In most cases I think the meaning of altering the context is pretty clear and
developers are already taking care of copying it when needed.

-- 
You received this bug notification because you are a member of OpenERP
Framework Experts, which is subscribed to the bug report.
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