Make `consider-iterating-dictionary` consider memb. check #4997

DanielNoord · 2021-09-13T19:47:17Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature, or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Type of Changes

	Type
✓	✨ New feature

Description

This closes #4069

The original issue talked about a new checker but I felt this fitted quite nicely within the description of the original checker.
I also removed a redundant check in the line before the message was added.

This closes pylint-dev#4069

tests/functional/c/consider/consider_iterating_dictionary.txt

coveralls · 2021-09-13T19:55:29Z

Pull Request Test Coverage Report for Build 1233871538

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.001%) to 93.083%

Totals
Change from base Build 1232904023:	-0.001%
Covered Lines:	13256
Relevant Lines:	14241

💛 - Coveralls

pylint/checkers/refactoring/recommendation_checker.py

tests/functional/c/consider/consider_iterating_dictionary.txt

Pierre-Sassoulas

👍 nice false negative fixed !

tests/functional/c/consider/consider_iterating_dictionary.py

DanielNoord · 2021-09-14T10:45:38Z

@Pierre-Sassoulas Any idea why your merge-commit made it fail the tests? Seems strange..

Pierre-Sassoulas · 2021-09-14T11:30:57Z

I don't know, we did not change anything for invalid-name on the main branch. This is very strange.

cdce8p · 2021-09-14T11:53:26Z

Really strange. I though a rebase might fix it, but apparently not 🤷🏻‍♂️
Tested the commits on my personal dev branch, and the invalid-name doesn't happen.

tests/functional/c/consider/consider_iterating_dictionary.py

pylint/checkers/refactoring/recommendation_checker.py

cdce8p · 2021-09-14T12:06:23Z

This seems to be caused by pylint-dev/astroid#979

…iterating

Pierre-Sassoulas

Maybe a false negative for invalid-name that was fixed in the latest version of astroid ? var is an invalid name (bad names list), so I think we can disable it or change var name to something else

Pierre-Sassoulas · 2021-09-14T12:24:58Z

tests/functional/c/consider/consider_iterating_dictionary.py

-# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods, no-member, import-error, no-self-use, line-too-long, useless-object-inheritance, unnecessary-comprehension
+# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods
+# pylint: disable=no-member, import-error, no-self-use, line-too-long, useless-object-inheritance
+# pylint: disable=unnecessary-comprehension, use-dict-literal


Suggested change

# pylint: disable=unnecessary-comprehension, use-dict-literal

# pylint: disable=unnecessary-comprehension, use-dict-literal,invalid-name

I think it would be better to change the name than add an additional disable here.
However, I don't see var in bad-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

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#979

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 the bad-names list from? I'm probably overlooking something..

Edit: Got it. It is because var is lowercase, uppercase fixes the issue. Pushing now!

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. Since Compare nodes are only just now inferred, these popped up.

Seems I mixed up invalid-name with disallowed-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. Thus invalid-name is emitted now.

Sorry for the ping @nelfin.

cdce8p

Some last comments

pylint/checkers/refactoring/recommendation_checker.py

cdce8p · 2021-09-14T13:29:56Z

pylint/checkers/refactoring/recommendation_checker.py

+        if (
+            isinstance(node.parent, (nodes.For, nodes.Comprehension))
+            or isinstance(node.parent, nodes.Compare)
+            and any(op for op, _ in node.parent.ops if op == "in")


It's a bit constructed, but technically possible. No warning should be emitted for

var = [1, 2] == {}.keys() in {False}

It should be enough to add an additional condition to the if clause, testing that the second value of ops is in fact the call node itself.

Some day I want to know as much about Python as you seem to do and be able to come up with these tests 😄

Thanks for the compliment ❤️

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

pylint/checkers/refactoring/recommendation_checker.py

Make consider-iterating-dictionary consider memb. check

400f8d7

This closes pylint-dev#4069

DanielNoord commented Sep 13, 2021

View reviewed changes

tests/functional/c/consider/consider_iterating_dictionary.txt Outdated Show resolved Hide resolved

cdce8p requested changes Sep 13, 2021

View reviewed changes

cdce8p added the Enhancement ✨ Improvement to a component label Sep 13, 2021

cdce8p added this to the 2.11.0 milestone Sep 13, 2021

Pierre-Sassoulas approved these changes Sep 14, 2021

View reviewed changes

tests/functional/c/consider/consider_iterating_dictionary.py Outdated Show resolved Hide resolved

DanielNoord added 2 commits September 14, 2021 09:05

Add additional tests and use-cases

82cd6dc

Split pylint disable's

0b40b49

Pierre-Sassoulas approved these changes Sep 14, 2021

View reviewed changes

cdce8p force-pushed the consider-iterating branch from 07889c8 to ebede01 Compare September 14, 2021 11:42

cdce8p reviewed Sep 14, 2021

View reviewed changes

tests/functional/c/consider/consider_iterating_dictionary.py Outdated Show resolved Hide resolved

pylint/checkers/refactoring/recommendation_checker.py Outdated Show resolved Hide resolved

DanielNoord added 2 commits September 14, 2021 14:10

Code review

556d627

Merge branch 'main' of https://github.com/PyCQA/pylint into consider-…

5a3b8e9

…iterating

DanielNoord force-pushed the consider-iterating branch from ebede01 to 5a3b8e9 Compare September 14, 2021 12:11

Pierre-Sassoulas reviewed Sep 14, 2021

View reviewed changes

Change var name to uppercase

46ade44

cdce8p reviewed Sep 14, 2021

View reviewed changes

DanielNoord and others added 2 commits September 14, 2021 15:33

Add additional check and test

6a77424

Update pylint/checkers/refactoring/recommendation_checker.py

aae8afc

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p reviewed Sep 14, 2021

View reviewed changes

Doc cleanup

d8bf54c

cdce8p approved these changes Sep 14, 2021

View reviewed changes

cdce8p merged commit af407c7 into pylint-dev:main Sep 14, 2021

DanielNoord deleted the consider-iterating branch September 14, 2021 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `consider-iterating-dictionary` consider memb. check #4997

Make `consider-iterating-dictionary` consider memb. check #4997

DanielNoord commented Sep 13, 2021 •

edited

Loading

coveralls commented Sep 13, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

DanielNoord commented Sep 14, 2021

Pierre-Sassoulas commented Sep 14, 2021

cdce8p commented Sep 14, 2021

cdce8p commented Sep 14, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas Sep 14, 2021

DanielNoord Sep 14, 2021

nelfin Sep 14, 2021

DanielNoord Sep 14, 2021 •

edited

Loading

nelfin Sep 14, 2021

cdce8p Sep 14, 2021

cdce8p left a comment

cdce8p Sep 14, 2021

DanielNoord Sep 14, 2021

cdce8p Sep 14, 2021

	# pylint: disable=unnecessary-comprehension, use-dict-literal
	# pylint: disable=unnecessary-comprehension, use-dict-literal,invalid-name

Make consider-iterating-dictionary consider memb. check #4997

Make consider-iterating-dictionary consider memb. check #4997

Conversation

DanielNoord commented Sep 13, 2021 • edited Loading

Type of Changes

Description

coveralls commented Sep 13, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1233871538

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Sep 14, 2021

Pierre-Sassoulas commented Sep 14, 2021

cdce8p commented Sep 14, 2021

cdce8p commented Sep 14, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `consider-iterating-dictionary` consider memb. check #4997

Make `consider-iterating-dictionary` consider memb. check #4997

DanielNoord commented Sep 13, 2021 •

edited

Loading

coveralls commented Sep 13, 2021 •

edited

Loading

DanielNoord Sep 14, 2021 •

edited

Loading