Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(4716): fix false positive unnecessary_dict_index_lookup emitted when del is used #5344

Merged
merged 7 commits into from
Dec 3, 2021
Merged

fix(4716): fix false positive unnecessary_dict_index_lookup emitted when del is used #5344

merged 7 commits into from
Dec 3, 2021

Conversation

yushao2
Copy link
Contributor

@yushao2 yushao2 commented Nov 19, 2021

Type of Changes

Type
✓ 🐛 Bug fix

Description

Add simple logic to check subscript's parent to see if it's Delete and ignore the rest check if so to prevent false positive emitted when performing index lookup for deletion of an dictionary entry

Closes #4716

@yushao2 yushao2 added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Nov 19, 2021
@coveralls
Copy link

coveralls commented Nov 19, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1536340558

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 93.694%
Totals Coverage Status
Change from base Build 1536319253: 0.007%
Covered Lines: 14160
Relevant Lines: 15113
💛 - Coveralls

DanielNoord
DanielNoord approved these changes Nov 19, 2021
View reviewed changes
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two valuable additions in quick succession 😄 One additional test but rest looks good!

We'll need to move the changelog for 2.13 again, but I'll add the blocked label to indicate this!

@DanielNoord DanielNoord added Blocked 🚧 Blocked by a particular issue False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Nov 19, 2021
@DanielNoord DanielNoord added this to the 2.13.0 milestone Nov 19, 2021
@yushao2 yushao2 removed the Blocked 🚧 Blocked by a particular issue label Nov 28, 2021
@yushao2 yushao2 requested a review from DanielNoord November 28, 2021 16:27
DanielNoord
DanielNoord reviewed Nov 28, 2021
View reviewed changes
tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt Outdated
unnecessary-dict-index-lookup:60:1:60:11::Unnecessary dictionary index lookup, use 'item[1]' instead:UNDEFINED
unnecessary-dict-index-lookup:65:10:65:20::Unnecessary dictionary index lookup, use 'item[1]' instead:UNDEFINED
unnecessary-dict-index-lookup:82:14:82:18::Unnecessary dictionary index lookup, use '_' instead:UNDEFINED
unnecessary-dict-index-lookup:7:10:None:None::Unnecessary dictionary index lookup, use 'v' instead:UNDEFINED
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you by any chance working on Python < 3.8?
The end line and end column information of these messages should be added here, they seem to have been transformed into None due to the auto-updater. This is probably also going to fail the test.

@Pierre-Sassoulas Shall we make the updater only accessible on > 3.7? Or make it put out a warning on lower versions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we need a run time error if the upgrade would be useless in CI. #5418 is ideal for that I guess ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Seems like a good idea. For now 3.8 is good enough since we don't have any nodes that only have their end_column data on 3.9 or 3.10. I think an exception when trying to run and a small explanation on why 3.8 is necessary should do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1e29198

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad let me switch my environments

@DanielNoord DanielNoord added the Blocked 🚧 Blocked by a particular issue label Nov 28, 2021
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Nov 28, 2021
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice light fix, thank you !

@DanielNoord DanielNoord mentioned this pull request Dec 3, 2021
Closed
@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Dec 3, 2021
@Pierre-Sassoulas Pierre-Sassoulas merged commit ef250da into pylint-dev:main Dec 3, 2021
@DanielNoord DanielNoord mentioned this pull request Dec 16, 2021
Closed
@yushao2 yushao2 deleted the fix-4716 branch January 4, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@DanielNoord DanielNoord DanielNoord approved these changes

Assignees
No one assigned
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

getting unnecessary-dict-index-lookup when deleting an item by value
4 participants
@yushao2 @coveralls @Pierre-Sassoulas @DanielNoord