openerp-community-reviewer team mailing list archive

[Joël Grand-Guillaume @ camptocamp <joel.grandguillaume@xxxxxxxxxxxxxx>]

Review: Needs Fixing code review, no test

Hi Yannick,


Thanks for this back port and I very welcome your first (if I'm not wrong) contribution here ! I've just a couple of remarks:

 * Line 66: For the author part, as it's OpenERP who did it first, I think it's fair to let OpenERP here. I suggest to add an author section in description of the __openerp__.py where you can explain you did the back port. This is also the new way we try to deal with authors in community module. 

 * Line 199: This method is quite long, and if I would like to override one, it's probably the one I would need to change. So I would suggest to add 2 methods: _prepare_invoice_vals (to fill inv_data) and _prepare_invoice_line_vals (to fill invoice_line_vals). With this anyone would easily override that part of the code. Now the question is : should we or shouldn't we change OpenERP's code as you took it from trunk.

 * In general, I also see that PEP8 is not really respected here, but well, I know you just take the code in the trunk.. So same question: should we improve that here, or better to let "as it is" in trunk.

 * No .pot file or i18n folder. Would be good to add it to enable the translation here.

Otherwise, it looks good to me. Would be happy to have other's opinion for 2nd and 3rd point.

Regards,

Joël



-- 
https://code.launchpad.net/~yannick-buron/contract-management/contract-management/+merge/204718
Your team OpenERP Community Reviewer/Maintainer is subscribed to branch lp:contract-management.