Hi Yogesh,
That's much better, now it's time for cleaning up :-)
Here are some remarks (based on the diff only, I haven't tested) :
- Please simplify all the calls to dict.get(). dict.get() returns None if the
key is missing, so there is no need for a default value if None is ok as a
result. For example the following is useless:
if values.get('key', False):
...
If the goal is to test that 'key' is in values and has a valid value, just use:
if values.get('key'):
...
And if the goal is only to know if the key is defined, test with "in":
if 'key' in values:
...
Typical example is: "if result.get(id, False) and result[id]" -> "if
result.get(id)"
- Please remove all tests on 'context' if it's not used (you know, the "if x is
None: x = {}"). No need for this if context is not used in the method, just
pass the received value to the next method. Only methods that actually look
into the context need to care. The same is true for other kwargs like "values",
which is unused in the default get() method.
I know we have a lot of old useless code like this, so don't hesitate to remove
when refactoring or fixing other things (same is true for previous point too).
- Line 23: any reason why you removed "list" from the types that are kept
unchanged? I don't have any example of function fields that return lists as
values for binary fields, but by removing it you are possibly introducing
side-effects, aren't you?
- sanitize_binary_value(): as this function now handles directly the values
instead of dict items, the parameter should be renamed to "value" or similar,
keeping "dict_item" is confusing.
- postprocess(): please avoid "type" as a parameter name, it is shadowing the
"type" keyword. You could use 'column_type' or similar.
- last, but not least: I'm probably missing something but I don't see the code
for postprocessing m2o values anymore? We still need to perform the name_get()
call for function fields that only return the IDs in the result, right? I guess
some function fields do it themselves already, but probably not all...
Thanks!
PS: I think "Resubmit" review type is used to request the submitter to make a
new merge proposal, for example on a different target branch. You can just post
a normal comment to notify reviewers that you finished fixing the code :-)
--
https://code.launchpad.net/~openerp-dev/openobject-server/trunk-bug-781247-ysa/+merge/61933
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-server/trunk-bug-781247-ysa.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
