Magento OpenERP Connector

Code review comment for lp:~camptocamp/magentoerpconnect/oerp61-oldstable-import-partners-2012-12-13

  1. oerp61-oldstable-import-partners-2012-12-13
  2. Merge into trunk_version
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

you may want to remove the FIXME tag at line 31 as you have fixed the issue ;-)

@nitpicking
l89. missing blank line
l93, l111 extra blank lines
The pep8 convention specifies 2 lines before classes and top level-functions and 1 single line between methods in class definition [0]

l112 'import_partners' should preferably be '_import_partners' because it can't be called from the outside (with XML/RPC for instance) due to the browse_record argument.

l158 useless parenthesis

l187 if website_id:
I don't think that it is possible to have a website_id which is 0, but if it could be the case we would preferably test if website is not None:

l189 mini typo s/an other/another/

l200
from A to B on a old account and create a new account with B address.
shouldn't it be?
from A to B on a old account and create a new account with A address.

Apart of these details (not all of them need to be changed, but please check the comment at l200), it sounds good to me, but I'll review the corresponding merge in extra-trunk and come back here.

[0] http://www.python.org/dev/peps/pep-0008/#blank-lines

« Back to merge proposal