openerp-dev-web team mailing list archive

["Xavier \(Open ERP\)" <xmo@xxxxxxxxxxx>]

I'll trust you for the bug fixing itself, or maybe you could spend an hour or two with Noz reviewing the bugs and checking that they're fixed?

For the tech review:

addons/openerp/controllers/actions.py:339
There is no need for the copy() in ctx.update(rpc.session.context.copy())
You could also do all of that in a single statement:

    ctx = dict(rpc.session.context, **(data.get('context') or {})

addons/openerp/controllers/fieldpref.py:87
Why are you testing for type() and then why are you checking for length?

The test could be replaced by 

    if not value and isinstance(value, basestring)

but I'd also wonder what types value can have. Is the type testing really necessary?

addons/openerp/controllers/form.py:274
Can't the second `kw.get('default_date')` be replaced by `kw['default_date']` since we already checked for presence?

addons/openerp/controllers/form.py:384
No need for the copy() in the ctx.update() call. Also, it's going not update params.context as well, is that normal?

addons/openerp/controllers/form.py:467
Too many copy() calls, this could be written

    ctx = dict((params.context or {}), **rpc.session.context)
    ctx.update(button.context or {})

no?

addons/openerp/controllers/form.py:993
Can't
    kind = ''
    if data.get(key):
        kind = data[key].get('type')
be rewritten:
    kind = data.get(key, {}).get('type', '')
?

addons/openerp/controllers/impex.py:71
is the setting of context to {} really necessary? Why?

addons/openerp/controllers/impex.py:116
ctx creation/setting should be a single call, redundant copy() call in ctx.update()

addons/openerp/controllers/impex.py:176
Use litteral_eval instead of eval, maybe set ctx = {} in the except clause? Can't you restrict the kind of exceptions caught by except?

Also, redundant copy() in ctx.update()

addons/openerp/controllers/impex.py:259
redundant copy of rpc.session.context in ctx.update()

addons/openerp/controllers/impex.py:292
is it really necessary to set context to {}?

Also, impex.py seems to have lots of unused variables (name and prefix in get_data for instance)

addons/openerp/controllers/impex.py::390
Redundant copy() of rpc.session.context, ctx creation could be performed in a single call

addons/openerp/controllers/impex.py:415
Is the intermediate `proxy` variable really useful?

Why not
    rpc.RPCProxy(params.model).fields_get(False, rpc.session.context)

addons/openerp/controllers/impex.py:449
Use append() to add a single item to a list, not += (or extend)

addons/openerp/controllers/search.py:120
You could use update() here to set both keys at once.

addons/openerp/controllers/translator.py:48
addons/openerp/controllers/translator.py:123
no need for the ctx = {} initialization, the next two lines could be a single call, redundant copy()

addons/openerp/utils/tools.py:156
line commented instead of removed

addons/openerp/utils/tools.py:194
What is the point of aliasing functions from os and os.path?

addons/openerp/utils/utils.py:243
    if val == []:
        values[key] = [(6, 0, [])]
    else:
        values[key] = [(6, 0, val)]

why not just `values[key] = [(6, 0, val)]

addons/view_graph/widgets/_graph.py:159
Wouldn't it have been simpler to test for `not value[x]`, set `res[x] = ''` in that case and bypass most of the complexity?


-- 
https://code.launchpad.net/~openerp-dev/openobject-client-web/backport-stable-trunk-bugfixes/+merge/26683
Your team OpenERP SA's Web Client R&D is requested to review the proposed merge of lp:~openerp-dev/openobject-client-web/backport-stable-trunk-bugfixes into lp:~openerp-dev/openobject-client-web/trunk-dev-web.