-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Make consider-iterating-dictionary
consider memb. check
#4997
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
400f8d7
Make ``consider-iterating-dictionary`` consider memb. check
DanielNoord 82cd6dc
Add additional tests and use-cases
DanielNoord 0b40b49
Split pylint disable's
DanielNoord 556d627
Code review
DanielNoord 5a3b8e9
Merge branch 'main' of https://github.com/PyCQA/pylint into consider-…
DanielNoord 46ade44
Change var name to uppercase
DanielNoord 6a77424
Add additional check and test
DanielNoord aae8afc
Update pylint/checkers/refactoring/recommendation_checker.py
DanielNoord d8bf54c
Doc cleanup
cdce8p File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 18 additions & 15 deletions
33
tests/functional/c/consider/consider_iterating_dictionary.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,18 @@ | ||
consider-iterating-dictionary:23:16::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:24:16::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:25:16::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:26:21::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:27:24::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:28:24::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:29:24::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:30:29::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:31:11::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:36:24::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:36:55::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:37:31::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:37:61::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:38:30::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:38:60::Consider iterating the dictionary directly instead of calling .keys() | ||
consider-iterating-dictionary:25:16::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:26:16::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:27:16::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:28:21::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:29:24::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:30:24::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:31:24::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:32:29::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:33:11::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:38:24::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:38:55::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:39:31::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:39:61::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:40:30::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:40:60::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:43:8::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:45:8::Consider iterating the dictionary directly instead of calling .keys():HIGH | ||
consider-iterating-dictionary:65:11::Consider iterating the dictionary directly instead of calling .keys():HIGH |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 it would be better to change the name than add an additional disable here.
However, I don't see
var
inbad-names
in./pylintrc
.Furthermore, this doesn't explain why this is only being thrown in the CI. The tests pass locally for me (and I think also for @cdce8p). Probably should try and investigate what is exactly causing this before adding a disable.
@cdce8p seems to have an idea, or at least has located the source
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 was able to replicate the failure when running under tox: (
tox -e py36 -- -k consider_iterating
). Sorry about jumping in, I was just pinged by the mention of pylint-dev/astroid#979Uh oh!
There was an error while loading. Please reload this page.
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 forgot to add
--recreate
, which you probably did some time before. Can now indeed reproduce.I can change this to another variable name. Can somebody tell me where
pylint
is getting thebad-names list
from? I'm probably overlooking something..Edit: Got it. It is because
var
is lowercase, uppercase fixes the issue. Pushing now!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.
As to why this shows up after that merge on astroid, my reading of https://github.com/PyCQA/pylint/blob/bc95cd34071ec2e71de5bca8ff95cc9b88e23814/pylint/checkers/base.py#L1975-L1984 is that
invalid-name
is only emitted if the Assign.value
can be inferred. SinceCompare
nodes are only just now inferred, these popped up.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.
Seems I mixed up
invalid-name
withdisallowed-name
myself.var
is fine to us, but here it's inferred as constant which should be uppercase (like @DanielNoord mentioned).pylint-dev/astroid#979 added additional inference for
compare
nodes which is why these can now correctly be inferred as constants. Thusinvalid-name
is emitted now.Sorry for the ping @nelfin.