Merge lp:~camptocamp/openobject-server/trunk-proposed-cleanups into lp:openobject-server

Why not keep the simpler import (from osv import orm, fields) and extend from orm.Model?

No special reason, that's closer to the style I spontaneously write. I have no strong feeling about this, though, so if it helps getting this proposal merged I can update the changeset, and take the opportunity to update it to the latest trunk head.

> Why not keep the simpler import (from osv import orm, fields) and extend from
> orm.Model?

Hello Xavier,

I've updated the merge proposal. I've also set you as reviewer for https://code.launchpad.net/~c2c/openobject-addons/trunk-proposed-cleanups/+merge/102832 which is very similar, but works on the addons rather than on the server.

Both changeset have been tested on the runbot and produce a "green" result, which is encouraging. I had to go a bit beyond the original scope of the patches to achieve this, as some problems were breaking the tests.

If these two MP are accepted (which I hope), it would be great if the message was sent internally and if the merging policy for trunk was updated to require the merged changeset to match this (use orm.Model, don't instantiate the model).

Thanks for your time.

> Why not keep the simpler import (from osv import orm, fields) and extend from
> orm.Model?

IMO refactor must include CamelCase for clases, i am agree to extend from orm.Model but we can:

from Osv import Orm, Fields

class MyObject(Orm.Model):
    pass

looks pythonic.

> > Why not keep the simpler import (from osv import orm, fields) and extend
> from
> > orm.Model?
>
> IMO refactor must include CamelCase for clases, i am agree to extend from
> orm.Model but we can:
>
> from Osv import Orm, Fields
>
> class MyObject(Orm.Model):
> pass
>
> looks pythonic.

@ovnicraft I'm totally opposed to renaming the module with non all-lowercase names (in other word, NO to "from Osv import Orm, Fields")

As for renaming class stock_picking to class StockPicking, I'm not sure. I could do this in another merge request, but I'd rather have this one integrated as soon as possible, because maintaining the patch branch against the trunk is quite costly.

updated my branch with latest trunk. Tests are green

For the record, extract from chat with Stéphane Wirtel:

(10:19:02) Stephane Wirtel: simple question, dans ton merge de lundi, tu utilises orm.Model au lieu d'osv.Model, tu as une raison ?
(10:32:18) alf: utiliser le module dans lequel la classe est définie
(10:32:59) alf: plutôt que osv qui ne fait qu'importer Model depuis orm
(10:33:45) alf: ça évite de casser tout le code le jour où on supprime l'import de Model dans osv.py parce qu'il n'est pas utilisé dans ce module
(10:34:58) alf: en général c'est une bonne pratique de ne pas se reposer sur ces imports "transitifs"
(10:35:22) alf: à part pour les trucs qui sont définis dans __init__.py parce que dans ce cas on peut penser qu'il s'agit de l'api du package

rough translation:

SW: simple question, in your MP you use orm.Model instead of osv.Model. Why?

ME: to use the module in which the class is defined (orm) rather than osv which is importing the class from orm. This avoids breaking all the code when Model is no longer imported in osv.py (e.g. because it is no longer used in that module). As a general rule, it is a good practice not to rely on these 'transitive' imports, apart from things which are imported in __init__.py because these can be considered as the package's API.

more details:

What I call a transitive import is A.py defines foo. B import foo from A (and does stuff with it). C import foo from B and does stuff with it. There are several problems with this:

1. changing the import in B can have unexpected side effects which are difficult to anticipate (B's unittest will probably still pass, but C's unittest will suddenly break)
2. (removing the import and) redefining foo in B can break C in subtle ways
3. when working on C, if you want to look for the definition of foo, you have to work your way through the whole import chain. Here we are dealing with a simple chain, but I've seen longer chains (sometimes with "import *" embedded for extra pleasure and it gets very messy very quickly)
4. this way of doing imports makes it easy to create import loops (and there are enough of these in openerp so far)

It would be quite different if the Model, etc... classes were exported by the osv package and not the osv module: in that architecture, the whole osv package's API would be well defined in the __init__.py file and all the submodules would be implementation details with which the common addon developper is not supposed to mess.

updated the branch to the latest trunk. tests are green

Reading the MP in its current state should give an idea of what the goal is. Maintaining this patch against trunk is time consuming, I'm going to stop doing so, but of course I'll gladly update the patch for a clean merge once I get a signal that the merge is going to happen.

Can you rebase on trunk since all imports have been cleaned

@al-openerp: Thanks for getting back on this. I'm doing this and making sure this branch and its friend on openobject-addons are green on runbot, and I'll ping you once this is in a mergeable state.

@al-openerp: this branch as well as its cousin branch on the openobject-addons project have been updated to a recent trunk version, and the runbot is green.

Just a little one that should be changed otherwise LGTM

191 +class ir_attachment(osv.Model):

@yvaucher-c2c: that's the case in the latest head, AFAICT

Really good job Alexandre,

I hope OpenERP will hear your proposal, otherwise OpenERP 26 will have a lot unvalid code

A lot of developer add yet class call in their module (nice module however). It is not useful since 2.5 years

http://bazaar.launchpad.net/~openerp-india/openerp-india/trunk/view/head:/quotation_template/quotation_template.py#L101

Please update your snippets and don't copy/paste

OpenERP is a really good solution, easy to understand/extend, I would like it will continue to be like up to 50 years further.

Perhaps only one solution => break compatibility shorter with older code

Thanks for all those which contribute to clean code

Can you rebase on trunk, think only the osv.osv -> osv.Model changes will remain.

@Alexandre l.191 is osv.Model instead of orm.Model my comment wasn't clear enough