Code review comment for lp:~openerp/openobject-client/property_attribute

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Hello Husen,

Thank you for the merge proposal(s). The implementation looks simple enough, however the upper/lower conversion doesn't look like it could work for non-ascii characters, which makes it a bit useless as OpenERP should be compatible with all kinds of alphabets.

But anyway even if that worked, I'm afraid we can't consider merging this at this point for 6.0, for a few reasons:

- there are no modules really needing this feature in the current official addons, so I think FP will not accept adding small features like this, as it just makes the system more complicated for no real added value (unless you know of a case where this is required in current addons?)

- forcing textfield content to uppercase/lowercase seems like it can be done more reliably at the model level, so that any write() to the model will be supported, not just those coming from a client that support these "properties".

- forcing textfield contents to numeric values looks more like it should be an integer field, as again it would be supported properly at model level

- finally, if we decided to implement this sort of special case at client-side, both clients would need to be modified to support it, and after RC2 it is too late to introduce such features.

Thanks a lot for taking the time to make the merge proposals, though.
I suppose you needed this feature for a customer project, and it perhaps makes sense to keep it for that project, but we cannot include it in the core at the moment.

I will close the proposals for now and perhaps we can re-consider this after release 6.0.

I hope you understand my point of view...

review: Disapprove

« Back to merge proposal