Skip to content

New checker unnecessary-list-index-lookup (#4525) #5834

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
Merged
merged 2 commits into from
Mar 25, 2022
Merged

New checker unnecessary-list-index-lookup (#4525) #5834

merged 2 commits into from
Mar 25, 2022

Conversation

@timmartin
Copy link
Contributor

@timmartin timmartin commented Feb 23, 2022

Type of Changes

Type
🐛 Bug fix
✓ ✨ New feature
🔨 Refactoring
📜 Docs

Description

Added a new checker for catching the case of iterating over a list using enumerate():

for index, value in enumerate(mylist):
    print(mylist[index]) # Bad, should use value instead

This will now report

Unnecessary list index lookup, use 'value' instead

Closes #4525

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Feb 25, 2022
@timmartin timmartin force-pushed the issue-4525-unnecessary-list-lookup branch from 8fc9d8c to 7e626dd Compare February 27, 2022 08:48
Pierre-Sassoulas
Pierre-Sassoulas requested changes Feb 27, 2022
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.

Thank you for opening the pull request, this look goods already. I think we'll be able to add this to 2.14, but we need to release 2.13 at some point and we're lagging behind all the contributions 😄. Could you move the changelog to 2.14 ? I know the "what's new" page for 2.14 is not created yet, sorry.

@timmartin timmartin force-pushed the issue-4525-unnecessary-list-lookup branch 4 times, most recently from 9502b59 to c7c0c51 Compare March 2, 2022 18:28
@coveralls
Copy link

coveralls commented Mar 2, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2042126251

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 94.184%
Totals Coverage Status
Change from base Build 2040724854: 0.009%
Covered Lines: 15337
Relevant Lines: 16284
💛 - Coveralls

DanielNoord
DanielNoord requested changes Mar 2, 2022
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.

This looks very good already!

I left some comments, let me know if you have any questions about them 😄

@timmartin timmartin mentioned this pull request Mar 2, 2022
Merged
@timmartin timmartin force-pushed the issue-4525-unnecessary-list-lookup branch 2 times, most recently from dd8f358 to 7076fe0 Compare March 4, 2022 22:42
DanielNoord
DanielNoord reviewed Mar 6, 2022
View reviewed changes
pylint/checkers/refactoring/refactoring_checker.py
# enumerate() result is being assigned without destructuring
return

iterating_object_name = node.iter.args[0].name
Copy link
Collaborator

@DanielNoord DanielNoord Mar 6, 2022

Choose a reason for hiding this comment

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

I'm wondering if there are edge cases to consider with len(args) > 1, but I think in those cases using the index is so unnecessarily complicated that most people will have moved to using value at that point. So I think we can leave those edge cases unconsidered here.

DanielNoord
DanielNoord approved these changes Mar 6, 2022
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.

Thanks a lot @timmartin, this looks good to me!

@Pierre-Sassoulas Depending on your schedule and free time, we might actually get this reviewed and merged before we release 2.11. No rush though!

@Pierre-Sassoulas Pierre-Sassoulas added Checkers Related to a checker Enhancement ✨ Improvement to a component labels Mar 6, 2022
@DanielNoord
Copy link
Collaborator

DanielNoord commented Mar 24, 2022

Hi @timmartin,

I know I already approved this PR, but...
We have been working on the documentation of the messages that pylint emits. As this PR adds a new message could I ask you to rebase on main and add two examples for bad and good code like explained in #5953? That way we immediately help users understand what this new message will do 😄

@timmartin timmartin force-pushed the issue-4525-unnecessary-list-lookup branch from 43dce52 to 0f794ae Compare March 25, 2022 20:30
@timmartin
Copy link
Contributor Author

timmartin commented Mar 25, 2022

I've rebased and added documentation now.

@timmartin timmartin force-pushed the issue-4525-unnecessary-list-lookup branch from 4fdb3ba to 207060d Compare March 25, 2022 20:32
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Mar 25, 2022
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.

Thank you for the two examples, looks great !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 8d21ab2 into pylint-dev:main Mar 25, 2022
@timmartin timmartin deleted the issue-4525-unnecessary-list-lookup branch April 27, 2022 20:53
@The-Compiler The-Compiler mentioned this pull request Jun 8, 2022
Closed
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

Checkers Related to a checker Enhancement ✨ Improvement to a component

Projects

None yet

Milestone

2.14.0

Development

Successfully merging this pull request may close these issues.

New checker unnecessary-list-index-lookup

4 participants

@timmartin @coveralls @DanielNoord @Pierre-Sassoulas