Review: Disapprove Sorry to interrupt, but I have a few remarks. (These remarks are quite generic and could apply for most developers and most commits, so do not take them personally :-)) :
1. "res" is a wrong and obfuscated name for a parameter, please change it to e.g. processed_modules. Why do you always call everything "res"? It is a very bad idea and forces everyone to read the code completely to know what "res" is supposed to be! Code is only written once but is read countless times, so it's worth spending 2 seconds to find a meaningful name. This is one of our most basic guidelines (2.2) in http://doc.openerp.com/v6.0/contribute/15_guidelines/coding_guidelines.html 2. @Naresh and Xavier: Is this even a bug? I'm not sure because the comments and explanations are far from clear, but it looks like an optimization of calculation speed when pressing the module install button... something that is suited for trunk but not for a stable branch. You had to patch the server for this and you changed the API (signature of the `state_update` method changed). These are 2 things that should be considered very carefully, especially if it is not even for a real bug, and only an "improvement"! "stable" does not mean "merge anything that customers ask for"! If there was really a bug then I take this point back, but it's really not clear when reading the patch and comment. 3. Please try to make your commit comments useful and self-descriptive: - do not simply say "fix OPW xxx" because it makes reviewing the commit/merge history impossible (I've seen this many times!) - please explain what you fixed and how, e.g. "Dependancies loop executes when you install a new module" does not say what is the problem and it does not say what you fixed! A "loop" is not a bug, every "for" statement is a loop. Something like "[IMP] Speed up dependency calculation by only processing each dependency once" would have described in one sentence what was the problem and how you fixed it. The above may seem like small details but in the long run it makes the difference between a bad developer who does not care about his work and a good one who can always be proud of his work and how his code and commits will be read by others. Do not take this personally :-) Thanks for paying attention to this! -- https://code.launchpad.net/~openerp-dev/openobject-server/6.0-opw-381612-ado/+merge/88682 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-server/6.0-opw-381612-ado. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
