Code review comment for lp:~pablocm/account-payment/7.0-account-payment-extension-fix-invoice-field

Revision history for this message
PabloCM (pablocm) wrote :

> Hi, Pablo, thank you very much for the fix, because it's very critical to get
> this thing fix, but there are some things you can fix:

Hi, np, it's the least I can do.

> - You don't need to redefine the field invoice. This field is already defined
> in account module and if you put the same method name, it will be got called
> by inheritance. If you overwrite the field, any change in the core in the
> future will not be reflected.

I know it is really bad to overwrite the field, shadowing future changes, but
I had to. If I don't, the new methods won't get called. Maybe it's only happening
in the instance I've tested it (openerp-7.0-20130806-231058) but are you sure it
works as you say?

> - You can make the renaming with another way: instead of staying with cursor
> and user variables and change signature, you can rename all the ocurrences to
> the more standarized cr and uid. This is no problem, because overriding
> methods, positional arguments names doesn't matter.

The parameters in the original function it is redefining are named that way and
I thought it would be better to have both with the same signature.
I'll change them to follow the standard as you suggest.

> - Why are you removing 'invoice_ids = []' line?

It was unused, why keep it?

> - Doesn't occur the same problem with arguments with _invoice_search?

As you say, there is no problem when having different names for positional arguments.
For consistency shake I should have renamed them, but since we are going to stick to
the standarized names -cr and uid- it is fine as it is.

Regards

« Back to merge proposal