Review: Resubmit
Thanks Quentin for your time,
my reply on your comments
> just few comments:
> 1) can you make a new function to retrieve the dict to use to create the
> invoice line,
> _prepare_invoice_line_data(), in order to increase the inheritability and
> flexibility of our code
I can not make a new function as inv_line_create function is used in other
modules too, and might be used somewhere else in other community modules. I
just changed signature of inv_line_create to return dict of data and this will
be less risky as it requires less changes in overridden method.
> 2) really strange: i didn't know that the field invoice_id on
> account.invoice.line wasn't mandatory! anyway, for performance reasons it
> would be better to create the invoice with no lines, then to create the lines
> with the invoice_id set.
I found its good "to create the invoice with no lines, then to create the lines
with the invoice_id set."
so I did that.
3) i don't like this syntax. Is there a reason to do it? a good one? if no, i
propose that you let it like that to keep the code as much consistent as
possible.
75 - self.write(cr, uid, [o.id], {'invoice_ids': [(4, inv_id)]})
78 + o.write({'invoice_ids': [(4, inv_id)]})
> 4) we need more comments, more docstrings in the new functions
Added!
> 5) some context may be added in function calls, from here to there, in the
> lines you already changed
true, I just tried to keep it more clean in order to focus on only fix, but
anyways I agree on that point and improved.
And as a bonus I changed stupid variable names a, o, il ..
this will cost you more to review but will be better, what do you think?
Thanks
--
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-bug-724131-rpa/+merge/82138
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-addons/trunk-bug-724131-rpa.
_______________________________________________
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
