openerp-community-reviewer team mailing list archive
-
openerp-community-reviewer team
-
Mailing list archive
-
Message #03989
Re: [Merge] lp:~omar7r/account-financial-tools/6_1_imp_acccount_admin_tools_importer into lp:account-financial-tools/6.1
Review: Needs Fixing code review, no tests
Please add contributors name in __openerp__.py under Contributors
And I think you can remove the "All Rights Reserved."
http://www.iusmentis.com/copyright/allrightsreserved/
PEP8 to fix
trailing white space
blank lines with white space
l.22 + l.28 + l.44 + l.48 ... please respect indentation standards
ll.75-76 you don't need to do this check here as you are not calling any dict method on context
ll.118-120 you can use parenthesis to wrap a long multi line condition thus you don't need '/'
l.151 + l.159 + l.166 you can remove '/' it has no use here as a string can be written on multiple line
l.191 missing context propagation on search
Global remark: Can you assign context using keyword? context=context sometimes it helps to avoid some issues with signature redefinition.
Thanks
--
https://code.launchpad.net/~omar7r/account-financial-tools/6_1_imp_acccount_admin_tools_importer/+merge/196281
Your team Account Core Editors is subscribed to branch lp:account-financial-tools/6.1.
References