Code review comment for lp:~therp-nl/account-financial-tools/6.1-account-purchase-default

On ven. 01 févr. 2013 10:43:42 CET, Ronald Portier (Therp) wrote:
> Stefan, I will take your performance tests in mind when reading single character (what about float and integer, I guess will be about the same?) fields in the future.
>
> Alexandre: thanks for your comments, with one exception I will use them to update the code.
>
> The exception is on the need to test for partner_obj... I never want to make assumptions in my code on the correctness of calling functions. So indeed, I think it is necesarry to handle the case of a non existing partner_id. Or the case that should be even more rare that a valid partner_id will still not result in a valid partner object. For instance when the connection with the database has been lost.
>
> The only thing is what to do about it.
>
> In most cases I would prefer to have an assertion error. partner_obj should exist. If it does not, it is most likely due to a programmer error. In this case, retrieving a default value for a column that a user can change anyway, I don't want to blow up - confront the user with an exception - when something goes wrong.

Well... I cannot force you :-)

IMO, if you have a supposedly valid partner_id and browsing
model['res.partner'] for that id returns a browse_null instance, not
doing anything is wrong, because you are silencing a bug (you should
not have gotten that partner_id in the first place). At the very least,
this should be logged. And in this specific case, displaying the
exception to the user would be not be inappropriate (because it should
not be happening anyway). Si at the very least if you want to guard
against this, use an assert statement.

--
Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 94

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com