Review: Resubmit
[DONE]
- all the select= are unnecessary in the views, to be removed.
- when a required=True is already present in the python definition, don't add
it again in the views.
- comment the ir_actions_todo_category and ir_actions todo classes would be
appreciated. Especially on the meaning of the new fields (Special, Normal,
Recurring).
- res.get('context', {}) can be simply res.get('context')
- put the self.pool.get outside the loops.
- in the _{get,set}_address_data: name should be a plural, arg is unused, a
docstring would be welcome.
- in the function fields, remove the method=True (a change in the server make
it unecessary).
- createReport and preview_report, why the (not)camel case? those methods seem
unused. and maybe they should belong to the reporting modules.
=> createReport removed. preview_report is used and cannot be removed
- service = netsvc.LocalService(report) outside the loop.
- report_file = '/tmp/reports'+ str(id) + '.pdf', is it safe? make use
something like mktempfile.
- can the Tocheck be made clearer?
- line 619, I tink you can do pop(x, False) instead of using an 'if' before.
What does-it do?
- if not len(ids) doesn't make sense as just after you test if ids is a single
number.
[DISAGREE]
- dict.update({'foo': bar}) can be dict.update(foo=bar)
=> unless dict.update(foo=bar) is faster, i don't see the point changing that.
I find that dict.update({'foo': bar}) is more explicit and it's also more
consistent with all our existing code.
- for the dict returned by the button_xxx methods: please define a single dict
D in one place and return dict(D, name='xxx').
=> is it really worth to create a variable to shaddow the returned value? I
find that a little more clear that way and besides who knows if the returned
value won't change for one of the button sooner or later?
- the string "Add More Features" should be "ADD MOAR FEATUREZ".
=> shouldn't that be "HAD MORE FIT YOURS" ?
- do you really want to call the field 'Street2' ?
=> well, it's the same label as in res.partner.address and i don't have a
better idea. If you have you can suggest. :-)
Thanks,
--qdp, OpenERP kung-fu panda team
--
https://code.launchpad.net/~openerp-dev/openobject-server/trunk-configuration-rework/+merge/66464
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-server/trunk-configuration-rework.
_______________________________________________
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
