← Back to team overview

openerp-community-reviewer team mailing list archive

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