credativ team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

Review: Needs Fixing

Hi Holger,

Thanks! Looks good, just a couple of comments:

openerp/modules/loading.py, line 46: it is a bad habit to instantiate mutable objects in function definitions. This creates constants, which are bound to the variables in calls to the function. Even if it does not matter here, it is good practice to initialize skip_modules as None in the function definition and then assign a sane default value in the function itself if None. See also section 2.6, "The infamous context" of [1].

openerp/openupgrade/openupgrade.py, line 160 to 162: importing from the 'openerp' namespace does not work in older versions. Please revert these lines so that this file can be kept the same between different versions of the OpenUpgrade server.

openerp/openupgrade/openupgrade.py, line 337: The 'version' argument that 'migrate' is called with takes a bit of an effort to predict (if at all possible) and it can also be empty. It is also not consistent in between different modules. For example, in the migration scripts for module 'hr' from 5.0 to 6.0, a legacy table (res_partner_function) is used that is shared with the base module in whose migration script it is renamed. Chances are that the 'hr' module's migration scripts are called with a different version argument than the base module's migration scripts. To retrieve the versioned legacy name of the table in the hr module, the versioning needs to be consistent between modules. Would you find it acceptable to use the global server version (that can be imported from version.py) instead of having the 'version' argument? If you think more complex situations can occur, you could allow for an optional argument for a custom, additional suffix.

Cheers,
Stefan.

[1] http://doc.openerp.com/v6.0/contribute/15_guidelines/coding_guidelines_framework.html
-- 
https://code.launchpad.net/~therp-nl/openupgrade-server/6.1-api_ports/+merge/109607
Your team OpenUpgrade Committers is subscribed to branch lp:openupgrade-server.