Code review comment for lp:~therp-nl/banking-addons/ba61-company_awareness_bank_import_settings

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

> Hi Alexandre,
> thank you for the style notes. I followed them up only with a slight variation
> in the return statements of _default_company(). Additionally, I removed the
> void company_id keyword argument to this method.

Thanks a lot for taking my contribution into account.

Indeed, no one seems to pass that argument...

As for the return statement, there are way too many "X and Y or Z" in openerp related code. I understand the historical context, the need for a single statement in many places. IMO when a single statement is not mandatory (e.g. in a return statement) the full blown if: else: construct should be prefered. In other case, I would like to see a generalisation of the use of Python's ternary operator (Y if X else Z) which does not feature the pitfall of "X and Y or Z" (which returns Z if X is true and Y is false). I'm not asking for this in my reviews unless I spot a case where Y can be false. Do you have an opinion on this?

« Back to merge proposal