[RFC] Ignore missing-return in compute methods

chienandalu commented 1 year ago

Would it be possible to ignore compute methods for the missing-return directive? They aren't meant to return anything and would avoid little absurds like this where the return is just superflous:

    discount = fields.Float(compute="_compute_discount", store=True)

    @api.depends("order_id", "order_id.general_discount")
    def _compute_discount(self):
        if hasattr(super(), "_compute_discount"):
            super()._compute_discount()
        for line in self:
            line.discount = line.order_id.general_discount
        return

moylop260 commented 1 year ago

It is not possible for a static checker

I mean, if you are inheriting a compute field method the field could not be defined in the class

I mean, the definition of the field could be

#repositoy1
class MetaClass...
    discount = fields...(compute="_compute_discount")

    def _compute_discount(self):
        ...

#repositoy2
class InhClass...
     _inherit = ...

    def _compute_discount(self):
        ...

The second class there are not a static way to know it is a method declared from fields compute attribute

Even the convention name could not be used since it not even is a fields compute method

See the following examples:

Notice the name starts with _compute and it has a return

What about returning the original value even if it is returning None?

Check the following cases returning the same original value:

I mean, in your example

    @api.depends("order_id", "order_id.general_discount")
    def _compute_discount(self):
        res = None
        if hasattr(super(), "_compute_discount"):
            res = super()._compute_discount()
        for line in self:
            line.discount = line.order_id.general_discount
        return res

Also, I think your code is a corner case since it inherits the original method but validating if it exists And it is overwriting the original value pre-computed in the original method

For these corner cases you can use a disable comment:

# pylint:disable=...

pedrobaeza commented 1 year ago

This follows the attempt of standardizing the inheritance converting fields to computed writable:

https://github.com/OCA/odoo-community.org/pull/50

moylop260 commented 1 year ago

if the field is re-defined in the same class, so it is possible to detect it for the static checker

(Note: The example code in the description of this issue doesn't have the field definition as part of the class in this case it is not possible to detect it)

pedrobaeza commented 1 year ago

Yes, the field should be redefined in the same class for converting it into compute.

chienandalu commented 1 year ago

Thanks for the explanations @moylop260 :slightly_smiling_face: :+1: We'll adapt to this then

pedrobaeza commented 1 year ago

Well, Moi is saying that it can be put the exception if the field is redefined in the same class, which is the case, so this may worth. Anyway, the debate about using this technique is still around.

moylop260 commented 1 year ago

I just updated the main description with the final and real case of use

Using this way it is possible to discard the lint

chienandalu commented 1 year ago

Well, Moi is saying that it can be put the exception if the field is redefined in the same class, which is the case, so this may worth. Anyway, the debate about using this technique is still around.

Oh, sure! I missed that part :slightly_smiling_face: Thanks for reopening Moi.

luisg123v commented 1 year ago

I don't find necessary to consider this case. Even though compute methods are generally not meant to return anything, I don't see why we should restrict that, and returning the super's result is always a good practice.

Regarding the getattr approach, since that technique is kind of a hack, the way to return would also be:

result = None
if hasattr(super(), "_compute_discount"):
    result = super()._compute_discount()
for line in self:
    line.discount = line.order_id.general_discount
return result

Or even:

result = getattr(super(), "_compute_discount", lambda self: None)()
for line in self:
    line.discount = line.order_id.general_discount
return result

Second alternative would even be better than original proposal, as it's better for coverage.

CC @antonag32

github-actions[bot] commented 1 year ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

OCA / pylint-odoo

[RFC] Ignore missing-return in compute methods #450