-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: avoid false positive when module-level names match class-level names #10723
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: main
Are you sure you want to change the base?
fix: avoid false positive when module-level names match class-level names #10723
Conversation
1a5cff6 to
178bdc8
Compare
Pierre-Sassoulas
left a comment
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.
Great first PR, thank you. Could you add a functional test in tests/functional to check it automatically, please ?
178bdc8 to
8f658a3
Compare
|
Thanks for the review @Pierre-Sassoulas, just added it! |
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
left a comment
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.
Made a suggestion to match the example from the issue, let me know what you think. Changed Input to input, maybe it was the reason for the second invalid-name (shouldn't have been pascal case ?)
| @@ -0,0 +1,8 @@ | |||
| """Test module-level constants with class attribute same 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.
| """Test module-level constants with class attribute same name""" | |
| """Test module-level constants with class attribute same name | |
| Regression test for #10719. | |
| """ |
| class MyClass: | ||
| MY_CONST = 10 |
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.
| class MyClass: | |
| MY_CONST = 10 | |
| class MyClass: | |
| MY_CONST = 10 | |
| class Theme: | |
| INPUT = ">>> " | |
| INPUT = Theme() | |
| input = Theme() | |
| OUTPUT = Theme() | |
| output = Theme() |
| @@ -0,0 +1,3 @@ | |||
| Fixed false positive for ``invalid-name`` (C0103) where module-level constants were incorrectly classified as variables when a class-level attribute with the same name exists. | |||
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.
| Fixed false positive for ``invalid-name`` (C0103) where module-level constants were incorrectly classified as variables when a class-level attribute with the same name exists. | |
| Fixed false positive for ``invalid-name`` where module-level constants were incorrectly classified as variables when a class-level attribute with the same name exists. |
(The reason is that we're going to change this to ``:ref: ìnvalid-name``` when we can handle it automatically in the changelog later, and this will print the (C0103) automatically, so less work later on.)
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.
Thanks! Comments applied
…ames - Add scope comparision to avoid module-level constants to be incorrectly classified as variables when a class-level attribute with the same name exists Closes pylint-dev#10719
8f658a3 to
6459607
Compare
|
There's lot of changes from the primer which is worrying. Also the tests are not passing you can launch them with pytest locally, see https://pylint.readthedocs.io/en/stable/development_guide/contributor_guide/tests/launching_test.html#pytest |
Description
Fixes a false positive where module-level constants were incorrectly classified as variables when a class-level attribute with the same name exists. I added a scope check so now when we check for reassignments, we make sure we only look at nodes within the same scope
Closes #10719