-
-
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
8f658a3 to
6459607
Compare
This comment has been minimized.
This comment has been minimized.
|
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 |
23c90df to
284a794
Compare
jacobtylerwalls
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.
Thanks! I checked a few of the primer changes, and they look good. Does anything look concerning to you?
pylint/checkers/utils.py
Outdated
| if isinstance(a, (nodes.ClassDef, nodes.FunctionDef)): | ||
| if a.parent and a.parent.scope() == node_scope: | ||
| return True | ||
| elif a.scope() == node_scope: |
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 think is is usually used for specific node comparisons.
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.
still applies
284a794 to
dc44856
Compare
jacobtylerwalls
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.
Looking good!
…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
dc44856 to
47b22a7
Compare
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (95.23%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10723 +/- ##
=======================================
Coverage ? 95.98%
=======================================
Files ? 176
Lines ? 19559
Branches ? 0
=======================================
Hits ? 18773
Misses ? 786
Partials ? 0
🚀 New features to boost your workflow:
|
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