fix(consider-interating-dictionary): fix false negatives #5331

yushao2 · 2021-11-17T17:43:39Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

Added loop logic to check upwards (parents of node) until i hit a compare node, to circumvent the issue where list(dict.keys()) results in the node.parent to be of type call

Also, added operator check for not in

Closes #5323

coveralls · 2021-11-17T17:49:55Z

Pull Request Test Coverage Report for Build 1513154202

7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.003%) to 93.507%

Totals
Change from base Build 1513087577:	0.003%
Covered Lines:	13998
Relevant Lines:	14970

💛 - Coveralls

DanielNoord

Thanks for this contribution!

We have currently closed 2.12 as we're trying to finalise that release, but this should definitely go in 2.13. We will need to update the place of the changelog entry when we can (after release of 2.12).

tests/functional/c/consider/consider_iterating_dictionary.py

ChangeLog

doc/whatsnew/2.12.rst

pylint/checkers/refactoring/recommendation_checker.py

tests/functional/c/consider/consider_iterating_dictionary.py

Pierre-Sassoulas · 2021-11-17T21:03:49Z

Nice to see a new contribution from you @yushao2 :)

yushao2 · 2021-11-18T10:28:27Z

Nice to see a new contribution from you @yushao2 :)

great to be back, had a hard time adjusting to my new role as devops but slowly getting the hang of it (and thus more room to breathe and to contribute :D)

DanielNoord

This is really quite elegant! Thanks for the quick work 😄

pylint/checkers/utils.py

pylint/checkers/refactoring/recommendation_checker.py

DanielNoord

Looks good except for the set membership test. Thanks for this!

Pierre-Sassoulas

Nice changes thank you @yushao2 ! I request change so we remember to move the changelog to 2.13 before merging.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

for more information, see https://pre-commit.ci

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Pierre-Sassoulas

Small change, big result 👍

DanielNoord · 2021-11-30T10:48:24Z

@yushao2 Note that this PR was/is blocked by the release of 2.12.2 and the Changelog entry in this PR is still in 2.12. It should either be 2.12.2 or 2.13. The other PR is also blocked!

Pierre-Sassoulas · 2021-11-30T10:49:08Z

Don't worry about this one, I'll fix the changelog when releasing 2.12.2

yushao2 · 2021-11-30T11:12:38Z

That is my bad, i got confused. Sorry for the trouble

cdce8p · 2021-12-02T14:02:23Z

pylint/checkers/utils.py

+def get_node_first_ancestor_of_type(
+    node: nodes.NodeNG, ancestor_type: Tuple[Type[T_Node]]
+) -> Optional[T_Node]:


Don't know if that has been considered. There is no need to limit ancestor_type to Tuple[...]. isinstance works fine with Type[T_Node] too. Ie.

ancestor_type: Union[Type[T_Node], Tuple[Type[T_Node]]]

This has the advantage that now you can pass both a single type and a tuple of types to it.

cdce8p · 2021-12-02T14:02:56Z

pylint/checkers/refactoring/recommendation_checker.py

@@ -83,21 +83,22 @@ def _check_consider_iterating_dictionary(self, node: nodes.Call) -> None:
            return
        if node.func.attrname != "keys":
            return
+        comp_ancestor = utils.get_node_first_ancestor_of_type(node, (nodes.Compare,))


comp_ancestor = utils.get_node_first_ancestor_of_type(node, nodes.Compare)

This could then be simplified.

Added to #5447 as a follow up issue to this.

I initially wanted to keep the typing of the function simple so thought it would be okay to only allow tuples, but we could allow both.

yushao2 force-pushed the fix--false-negative-5323 branch 2 times, most recently from 155ce7c to 08884a6 Compare November 17, 2021 17:57

yushao2 added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Nov 17, 2021

yushao2 requested a review from Pierre-Sassoulas November 17, 2021 18:05

DanielNoord requested changes Nov 17, 2021

View reviewed changes

DanielNoord added Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code labels Nov 17, 2021

DanielNoord added this to the 2.13.0 milestone Nov 17, 2021

Pierre-Sassoulas reviewed Nov 17, 2021

View reviewed changes

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

DanielNoord requested changes Nov 18, 2021

View reviewed changes

DanielNoord approved these changes Nov 19, 2021

View reviewed changes

DanielNoord added Blocked 🚧 Blocked by a particular issue and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Nov 19, 2021

Pierre-Sassoulas requested changes Nov 19, 2021

View reviewed changes

yushao2 and others added 7 commits November 29, 2021 00:25

fix(consider-interating-dictionary): fix false negatives

be257fe

Apply suggestions from code review

2b3ed7e

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

added util functions and changed code according to pr comments

554801a

Apply suggestions from code review

ccd2974

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

update code based on code review

507c502

[pre-commit.ci] auto fixes from pre-commit.com hooks

0ec1975

for more information, see https://pre-commit.ci

Update pylint/checkers/refactoring/recommendation_checker.py

39fb9eb

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

yushao2 force-pushed the fix--false-negative-5323 branch from 362e280 to 7dab24d Compare November 28, 2021 16:26

yushao2 added Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Blocked 🚧 Blocked by a particular issue labels Nov 28, 2021

chore(*): moved changelog entry from 2.12 to 2.13

03bae75

yushao2 force-pushed the fix--false-negative-5323 branch from 7dab24d to 03bae75 Compare November 28, 2021 17:16

DanielNoord approved these changes Nov 28, 2021

View reviewed changes

DanielNoord added Blocked 🚧 Blocked by a particular issue and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Nov 28, 2021

Pierre-Sassoulas approved these changes Nov 28, 2021

View reviewed changes

yushao2 merged commit 6caab13 into pylint-dev:main Nov 30, 2021

DanielNoord modified the milestones: 2.13.0, 2.12.2 Nov 30, 2021

DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Nov 30, 2021

DanielNoord mentioned this pull request Nov 30, 2021

Deprecate is_inside_lambda from utils #5447

Merged

2 tasks

cdce8p reviewed Dec 2, 2021

View reviewed changes

yushao2 deleted the fix--false-negative-5323 branch January 4, 2023 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(consider-interating-dictionary): fix false negatives #5331

fix(consider-interating-dictionary): fix false negatives #5331

yushao2 commented Nov 17, 2021

coveralls commented Nov 17, 2021 •

edited

Loading

DanielNoord left a comment

Pierre-Sassoulas commented Nov 17, 2021

yushao2 commented Nov 18, 2021

DanielNoord left a comment

DanielNoord left a comment

Pierre-Sassoulas left a comment

Pierre-Sassoulas left a comment

DanielNoord commented Nov 30, 2021 •

edited

Loading

Pierre-Sassoulas commented Nov 30, 2021

yushao2 commented Nov 30, 2021

cdce8p Dec 2, 2021

cdce8p Dec 2, 2021

DanielNoord Dec 2, 2021

fix(consider-interating-dictionary): fix false negatives #5331

fix(consider-interating-dictionary): fix false negatives #5331

Conversation

yushao2 commented Nov 17, 2021

Type of Changes

Description

coveralls commented Nov 17, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1513154202

💛 - Coveralls

DanielNoord left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented Nov 17, 2021

yushao2 commented Nov 18, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Nov 30, 2021 • edited Loading

Pierre-Sassoulas commented Nov 30, 2021

yushao2 commented Nov 30, 2021

cdce8p Dec 2, 2021

Choose a reason for hiding this comment

cdce8p Dec 2, 2021

Choose a reason for hiding this comment

DanielNoord Dec 2, 2021

Choose a reason for hiding this comment

coveralls commented Nov 17, 2021 •

edited

Loading

DanielNoord commented Nov 30, 2021 •

edited

Loading