Comment 23 for bug 525808

Revision history for this message
Raphaël Valyi - http://www.akretion.com (rvalyi) wrote : Re: [Bug 525808] Re: Default values should be immutable

Guys, sorry, if that's a silly question, but will that script be part of the
automatic quality checking in the CI server http://test.openobject.com ? I
think it would be better be part of it, is that planned?

Raphaël Valyi
http://www.akretion.com

2010/3/10 Numérigraphe <email address hidden>

> ** Description changed:
>
> Often in the code base we have:
> def foo(...., context={},....):
> ....
> As explained in comment #1, this should be changed to
> def foo(...., context=None,....):
> - if not context: # test incorrect, see below
> + # 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.
>
> --
> Default values should be immutable
> https://bugs.launchpad.net/bugs/525808
> You received this bug notification because you are subscribed to
> OpenObject Addons.
>
> Status in OpenObject Addons Modules: Confirmed
> Status in OpenObject Addons extra-trunk series: New
> Status in OpenObject Addons trunk series: Confirmed
> Status in OpenObject Server: New
> Status in OpenObject Server trunk series: New
>
> 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.
>
>
>
>
>