Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convention for the method name of new API onchanges #171

Open
pedrobaeza opened this issue Jan 20, 2016 · 5 comments
Open

Convention for the method name of new API onchanges #171

pedrobaeza opened this issue Jan 20, 2016 · 5 comments

Comments

@pedrobaeza
Copy link
Member

I have came across a problem that leads me to write this issue to discuss with the contributors: new API allows to set multiple onchange without the need of calling super to respect inheritance chain. This overcomes a past limitation with old-style view onchanges (the other is the no need of declaring parameters for accessing the values).

But this only happens, if the name of the method is different on each model overwriting!! I think this is a limitation in the way ORM handles pointers to these methods, only storing the name of the method in a list or a dictionary. Maybe @rco-odoo can give us a more detailed explanation or even a possible solution as a patch.

In the case this can't be solved, as we all tend to normalize the onchange method to onchange_<field_name>, which can lead to incompatible modules that both use the onchange with the same method name, one possible solution is to have as convention to name the methods as:

def onchange_<field_name>_<module_name>

This way, onchange methods will never collide and all will be executed.

What do you think?

@rvalyi
Copy link
Member

rvalyi commented Jan 20, 2016

will do the trick. At first I thought changing these onchange name for standardizing was really invasive. But then I thought at least we don't need to have them in the XML of the views anymore so that wouldn't require lot's of database update right? Only restart, correct? If so seems acceptable.

But still this looks like a basic limitation of the ORM is making us deal with hundreds of side effects in all the modules... I tend to think the ORM would better be fixed at all cost then, for me even if it was only in OCB as I don't really care anymore what SA does or not on its own suicide branch...

@moylop260
Copy link
Contributor

@pedrobaeza
Thanks for information.

Do you have a issue in odoo/odoo for this case?

We can justify this guideline with the answer of issue.

@pedrobaeza
Copy link
Member Author

I don't put any issue to Odoo because it cannot be considered like that, but a feature. I ping Raphael Collet for having his opinion in this issue instead of splitting the debate across multiple issues.

@rco-odoo
Copy link

The convention you propose looks perfectly fine to me.

The other proposal (normalize on onchange_<field_name> and make the ORM collect them all) looks bug prone and breaks Python conventions. It makes the ORM reinterpret method overriding. What if you actually want to override an existing onchange method?

@pedrobaeza 's convention could be summarized as:

  • to add an onchange method on field, define a method onchange_field_module decorated with @onchange; the naming convention avoids accidental name clashes
  • to modify an existing onchange method, override it like any Python method

@pedrobaeza
Copy link
Member Author

Thanks for the answer, Raphael. Yeah, it makes totally sense to respect Python overriding convention. I'll make the PR with the convention.

@hparfr hparfr mentioned this issue Dec 29, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants