-
Couldn't load subscription status.
- Fork 10.5k
[IMP] orm: add warning about model extension #14979
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
base: 19.0
Are you sure you want to change the base?
Conversation
|
@robodoo r+ |
|
@emi-odoo you can't review+. |
|
@kmagusiak I |
I cannot. @Julien00859 or @HydrionBurst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make another commit to fix the indentation, so we can backport that commit? Also we can't r+ it, we need someone from the documentation team
|
@Julien00859 you mean the indentation for the code examples of inheritance? If yes: will do 🫡 |
547e499 to
a485530
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello there @odoo/be-doc-review 👋 this PR is ready :)
c589c3d to
4fdcc4e
Compare
|
@Julien00859 -> I force pushed as I needed to resplit the work on the two commits (one for the warning and one for the code examples) |
| the attribute itself or by the class name (i.e: ``class MyModel(models.Model)`` will be | ||
| translated into ``_name = 'my.model'``, even if not explicitly set) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we raise a warning _logger.warning("Class %s has no _name, please make it explicit: _name = %r", name, attrs['_name']) when infer model name from class name. So I think developers shouldn't use this feature. And we don't have to document it here which increase the complexity.
| the attribute itself or by the class name (i.e: ``class MyModel(models.Model)`` will be | |
| translated into ``_name = 'my.model'``, even if not explicitly set) | |
| In all the other cases you need to explicity set the :attr:`~odoo.models.Model._name`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had developers being confused anyways because in the code example we're using _inherit = [...] (at least before this PR we were). And they didn't notice the class name. For new devs I think it's easy to get confused of the relationship between the class name and the model name and the table name. People will try to do it even if they don't realize what they're doing, so, from my point of view, it's better to explicitly explain what's going on in case they make a mistake :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| the attribute itself or by the class name (i.e: ``class MyModel(models.Model)`` will be | |
| translated into ``_name = 'my.model'``, even if not explicitly set) | |
| .. warning:: When :attr:`~odoo.models.Model._inherit` is set to a string, | |
| then :attr:`~odoo.models.Model._name` is set to the same value, | |
| unless `_name` is explicitly set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would simplify everything, the description of inhertance is also not accurate.
Inheritence
_name is not any _inherit
_name = 'a'
_inherit = 'b'
_name = 'a'
_inherit = ['b']
_name = 'a'
_inherit = ['b', 'c']
Extension
_name is one of _inherit and doesn't have to be specified only when _inherit is a str
_inherit = 'a'
_name = 'a'
_inherit = 'a'
_name = 'a'
_inherit = ['a']
Delegation
_inherits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but maybe this could be for another task/PR, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, it is already much better than before.
BTW, I think the message shouldn't be a warning 🤔
And the Note in the next line is for the content {'name': "A", 'description': "Extended"} in the previous line.
So we shouldn't add this content in between.
I would do
{'name': "A", 'description': "Extended"}. # It will also yield .....
Note: your new content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HydrionBurst a note would suffice! After the rephrasing especially :D
will check the format this evening :D
When a model is defined with `_inherit` using a list, Odoo will do a check with the class name to set the `_name` of the model being extended. This is different than the previous versions and can be misleading
This commit fixes the current indentation of python blocks, as they're not valid.
4fdcc4e to
6a5aacf
Compare
|
@Julien00859 @HydrionBurst -> adapted using JUC's suggestion! I feel like we still have the chance to confuse devs but better than before :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

When a model is defined with
_inheritusing a list, Odoo will do a check with the class name to set the_nameof the model being extended. This is different than the previous versions and can be misleading