Code review comment for lp:~camptocamp/openobject-addons/trunk-refactor-po-merge-lep

On 05/07/2014 08:59 AM, Leonardo Pistone - camptocamp wrote:
> Hi Olivier,
>
> in general I agree with your remarks! I probably won't be able to fix that
> week, but I should next week. Let's just keep in touch so we don't do the
> same job twice :)

Sure, thanks!

> - method signatures: a bit of a long discussion, and probably useless with
> the new API. Passing class instances instead of ids allows to easily write
> (isolated) unit tests like test_merge_origin_and_notes in this module. Here
> a mock is used, but the same approach can work with a real class instance.
> The same test for a method that accepts ids is more complex.

Oh, perhaps I wasn't quite clear in my previous comment. I didn't mean you
should change your methods to use "ids" instead of browse_records, that's fine.
I meant that you could still pass the cr, uid and context (not the ids!) to
your methods, even if you don't use them. This does not make testing/mocking
the methods any harder, and provides a more flexible/friendly extension point
for other modules using the old API.
This would be similar to what we do in the typical _prepare* methods in other
modules, also working with browse_records, e.g.:
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/10034/purchase/purchase.py#L479
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/10034/sale/sale.py#L348

Whenever the `purchase` module gets ported to the new API those parameters
would indeed become irrelevant, but not all official modules will be ported to
the new API at once. And theoretically the backwards-compatibility layer should
allow cross-API method overriding, by working out the cross-API calls.

I hope this makes sense to you,