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

Don't emit super-init-not-called for Enum subclasses #7181

Closed
wants to merge 1 commit into from

Conversation

AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Jul 13, 2022

Type of Changes

Type
🐛 Bug fix

Description

For some reason, commit 83d544b to cpython added a __init__
to Enum which does nothing (it just says pass). The examples
in the Enum docs:
https://docs.python.org/3.11/howto/enum.html
still do not include calling super's __init__ for Enum
subclasses, that define their own __init__, and obviously there
is no practical point to calling a method that does nothing, so
it seems best just to skip this warning for Enum.

Signed-off-by: Adam Williamson [email protected]

Blocked by python/cpython#30582 (comment)

@AdamWill
Copy link
Contributor Author

This is split out from #7167 , as requested by @DanielNoord .

@Pierre-Sassoulas Pierre-Sassoulas added Blocked 🚧 Blocked by a particular issue Maintenance Discussion or action around maintaining pylint or the dev workflow python 3.11 labels Jul 13, 2022
@AdamWill
Copy link
Contributor Author

Uh, I guess that line needs to be changed on main so the bot doesn't add it to every single PR?

@coveralls
Copy link

coveralls commented Jul 13, 2022

Pull Request Test Coverage Report for Build 2665599035

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 95.374%

Totals Coverage Status
Change from base Build 2665556819: 0.0005%
Covered Lines: 16802
Relevant Lines: 17617

💛 - Coveralls

@Pierre-Sassoulas
Copy link
Member

Uh, I guess that line needs to be changed on main so the bot doesn't add it to every single PR?

Thanks for noticing that's my bad.

@github-actions

This comment has been minimized.

For some reason, commit 83d544b to cpython added a `__init__`
to `Enum` which does nothing (it just says `pass`). The examples
in the Enum docs:
https://docs.python.org/3.11/howto/enum.html
still do not include calling super's `__init__` for Enum
subclasses, that define their own `__init__`, and obviously there
is no practical point to calling a method that does nothing, so
it seems best just to skip this warning for Enum.

Signed-off-by: Adam Williamson <[email protected]>
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit d076cc6

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thank you!

@Pierre-Sassoulas can we remove the blocked label here? This doesn't depend on 3.11 support, IMO.

@@ -2059,6 +2059,11 @@ def _check_init(self, node: nodes.FunctionDef, klass_node: nodes.ClassDef) -> No
except astroid.InferenceError:
continue

# Skip if klass is Enum, Python's own docs and examples
# do not recommend Enum subclasses call Enum.__init__
if klass.name == "Enum":
Copy link
Member

Choose a reason for hiding this comment

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

I think elsewhere we check the qname to avoid getting red herrings from other modules.

Suggested change
if klass.name == "Enum":
if klass.qname() == "enum.Enum":

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Jul 31, 2022
@Pierre-Sassoulas
Copy link
Member

It was blocked by python/cpython#30582 (comment) I think

@jacobtylerwalls jacobtylerwalls added this to the 2.15.0 milestone Jul 31, 2022
@AdamWill
Copy link
Contributor Author

@DanielNoord suggests that #7227 would likely avoid the need for this.

@Pierre-Sassoulas
Copy link
Member

A functional test to make sure of that would be nice :)

@DanielNoord
Copy link
Collaborator

@AdamWill Since #7227 was merged, would you be open to creating the functional test Pierre mentioned? That would indeed still be valuable.

@DanielNoord
Copy link
Collaborator

Going to close this as we actually have various regression tests against this throughout our code. Thanks for the initial investigation @AdamWill!

@AdamWill
Copy link
Contributor Author

No problem. Sorry I didn't get around to writing any tests, have had too many piles of things on fire to deal with :|

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.15.0 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow python 3.11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants