Code review comment for lp:~acsone-openerp/account-financial-tools/account_partner_required-sbi

Revision history for this message
St├ęphane Bidoul (Acsone) (sbi) wrote :

Hello Lorenzo,

About the constraints. This module is heavily inspired by account_analytic_required which used the same technique. I must confess I did not question the approach. Initially account_analytic_required was working mainly on the vals dictionary so it would have been faster than constraints. While working on account_partner_required I identified a bug in account_analytic_required which required a refactoring [1] which possibly rendered the optimization less obvious. Now since there are really two constraints to be checked based on the policy, the current approach is probably still slightly faster.

Would you agree to keep the current approach and postpone a possible performance test / refactoring with constraints to another MP?

Regarding yml vs unittest, it's only a matter of personal taste in this case. I notice the unittest2 framework seems better documented [2]. But hey, at least I have tests, right :)


« Back to merge proposal