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

[FIX] Exclude missing-return checks for computed methods #455

Closed
wants to merge 1 commit into from

Conversation

antonag32
Copy link
Contributor

Fixes #450. Compute methods are not supposed to return any value, even if there is a super() in it, since super() shouldn't return anything either.

The only reliable way to ensure a field is computed is if has been declared in the same python module (file), so this exclusion only applies to a limited set of cases.

@antonag32
Copy link
Contributor Author

@luisg123v can you review? thanks

# compute method names must be stored to avoid false positives if super is called inside them
if self.linter.is_message_enabled("missing-return"):
if hasattr(node, "keywords"):
for keyword in node.keywords:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for keyword in node.keywords:
for keyword in getattr(node, "keywords", []):

That way, you don't need the above conditional, so you may avoid an extra indentation level.

Or in fault, combine the two conditionals above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice, I made the change and also added a break once the compute attribute is found since there is no use in looking for a second one AFAIK.

Fixes OCA#450. Compute methods are not supposed to return any value,
even if there is a super() in it, since super() shouldn't return
anything either.

The only reliable way to ensure a field is computed is if has
been declared in the same python module (file), so this exclusion
only applies to a limited set of cases.
Copy link
Contributor

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented on the issue, I really don't consider this fix necessary. Always returning super is a good practice and there are other alternatives to sort the onchanges case.

@antonag32 antonag32 closed this May 24, 2023
@luisg123v luisg123v deleted the anton-450 branch May 24, 2023 02:31
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

Successfully merging this pull request may close these issues.

[RFC] Ignore missing-return in compute methods
2 participants