Code review comment for lp:~vauxoo/openerp-web/7.0-missing-menu-attr-nhomar

Hi,

This patch is unfortunately similar to some "hooks" that are often requested, like adding an empty method called "_hook_xxx" that does nothing, just to be able to override it.

We have to decline these requests because:
- it creates artificial legacy/unused cruft in the code, that is very likely to be removed in a future cleanup - breaking anything that depends on it
- it creates more confusion for the reader: What was the purpose of reading those values if they're unused? Should someone now report a bug now because the "web_icon" is not used in the web client?

Similarly, reusing a deprecated field (web_icon, etc.) for a new purpose is likely to get broken in the future when we cleanup, even if I can see why it is tempting for you to do it - as <menuitem> supports this attribute directly.

If you still want to do it and are aware of the consequences, you could perhaps make a patch to add a MENU_FIELDS_TO_LOAD constant that you could easily monkey-patch in your module, and would be less likely to break in the near future. At least this code would not scream "please clean me up" ;-)

I hope you understand my point..

Thanks!

PS: some of the attributes in the RNG are deprecated, we should consider removing them in trunk and cleaning up (what would `string` do, for example?)