openerp-community-reviewer team mailing list archive

["Holger Brunn \(Therp\)" <hbrunn@xxxxxxxx>]

Review: Needs Fixing

Thanks for your efforts!

I think what you did is perfectly fine, but the names and docstrings of the methods you introduce need improving: They pretend to return an id, while in fact they return a list of ids (which very often only has one element). So I'd rename the methods to get_tax_ids_or_default etc. and update the docstring accordingly. Sounds like nitpicking? I don't think so, for API functions it's very important that you get what you expect, and with a name phrased in singular, you expect a scalar, for a plural, you expect an iterable. And the fact that this doesn't apply to OpenERP's method/field names counts against their choice of names in the early days that we still have to live with now for legacy reasons, not for continuing doing this where it's not necessary.

Another thing is that I doubt '_or_default' needs to be part of the name. That should rather be part of the docstring.

Then if get_{supplier_,}taxes_or_default also accepted a list of ids, we could call it from a product's browse record, condensing the code a lot while making it more readable. Maybe add an assertion that the list can only contain one element.
-- 
https://code.launchpad.net/~lmi/ocb-addons/7.0-bug-796570-amu/+merge/203378
Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons.