openerp-community team mailing list archive

["Olivier Dony \(OpenERP\)" <odo@xxxxxxxxxxx>]

Review: Needs Fixing

Hi Stefan,

I agree that silently ignoring unknown fields in create() and write() can be a source of errors, especially while creating new module and debugging, so it would be a good practice to enforce that.

However, I can also imagine many cases where this situation is abused at the moment, because it makes the code simpler (e.g. when working with "virtual" columns), or just because it was overlooked. Indeed it would be better to fix 100% of these cases (and raising an exception would help that), but that would also mean a possibly great number of fixes will be needed in many modules, both community and core. While useful, those fixes are mostly nice-to-have, and are not likely to fix real bugs in existing modules.
So in the current case, I think logging a WARNING message would be better as a first step. That would still help for developing/debugging, and allow a progressive cleanup of existing abuse cases without breaking too much stuff. Maybe you could even use warnings.DeprecationWarning to indicate that in a future version this may indeed be completely forbidden.

Some other minor remarks:
- for consistency, read()/_read_flat() should do the same w.r.t. to unknown fields
- in error messages, why not mention directly all unknown fields, rather than just the first one? Log messages must not be translated so using different messages will not be a concern anymore, if it's easier.

The cleanup of XML files is very welcome, too!

Many thanks for the clean merge proposal, as always.

PS: I'll change the merge prop status to "Work in Progress", do not forget to change it back when you request another review - this is to help us sort out the numerous merge props, as review comments do not directly allow this.
-- 
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.