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
false positive unnecessary-list-index-lookup
for enumerate
#7685
Conversation
Pull Request Test Coverage Report for Build 3452558221
π - Coveralls |
This comment has been minimized.
This comment has been minimized.
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.
Thank you for the fix @clavedeluna. I think we can handle the false negative with start=0 too.
doc/whatsnew/fragments/7682.bugfix
Outdated
@@ -0,0 +1,3 @@ | |||
``unnecessary-list-index-lookup`` will not be emitted if ``enumerate`` is called with a `start` arg or kwarg. |
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.
``unnecessary-list-index-lookup`` will not be emitted if ``enumerate`` is called with a `start` arg or kwarg. | |
``unnecessary-list-index-lookup`` will not be wrongly emitted if ``enumerate`` is called with ``start``. |
for idx, val in enumerate(series, start=2): | ||
print(my_list[idx]) |
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.
for idx, val in enumerate(series, start=2): | |
print(my_list[idx]) | |
for idx, val in enumerate(series, start=2): | |
print(my_list[idx]) | |
for idx, val in enumerate(series, 2): | |
print(my_list[idx]) | |
for idx, val in enumerate(series, start=0): | |
print(my_list[idx]) # [unnecessary-list-index-lookup] | |
for idx, val in enumerate(series, 0): | |
print(my_list[idx]) # [unnecessary-list-index-lookup] |
`enumerate([1,2,3], 1)` | ||
""" | ||
if len(node.iter.args) > 1: | ||
# We assume the second argument to `enumerate` is the `start` arg. |
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.
# We assume the second argument to `enumerate` is the `start` arg. | |
# We assume the second argument to `enumerate` is the `start` arg. | |
# It's a reasonable assumption for now as it's the only possible argument: | |
# https://docs.python.org/3/library/functions.html#enumerate |
f9ba40a
to
d6d72e2
Compare
|
||
return not start_val == 0 | ||
|
||
return False |
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 actually tried to refactor this method because it looks like a bunch of duplicate code, but:
- refactoring wouldn't allow for some short-circuting which seems best for performance
- args v. kwargs has sliiightly different needs,
.value.operand
v..operand
etc
good call, I also went ahead and added calls for negative start value which made the logic more robust albeit a bit more complicated |
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.
Great ! π
The failing test on python 3.11 seems genuine |
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.
LGTM, thank you !
262105f
to
09c3ee9
Compare
09c3ee9
to
40edf3d
Compare
primer doesn't wanna prime π |
@DanielNoord would you be able to provide a bit more info on the exception? was there a stack trace? Or maybe something I could minimally reproduce and add a test for? Great catch! No wonder my other PRs have no issues with the primer |
It's actually quite easy to reproduce this yourself locally. Just run: python tests/primer/__main__.py prepare --clone
python tests/primer/__main__.py run --type=pr It will take some time to get to |
glad I opened #7714 |
92c3d3e
to
c9b98e4
Compare
Thanks @DanielNoord for getting me to finally run the primer locally. I must've done too many of the CI steps and got stuck initially. The error was easy to spot and fix, so I hope everything passes now! |
π’ primer, I don't think it has anything to do with this PR |
A case study for #7738 |
c9b98e4
to
fe33027
Compare
This comment has been minimized.
This comment has been minimized.
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.
Small nitpick, it's ready to go otherwise, nice fix @clavedeluna π
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit bf59f13 |
* do not report unnecessary list index lookup if start arg is passed * account for calling start with 0 or negative num Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
* do not report unnecessary list index lookup if start arg is passed * account for calling start with 0 or negative num Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Type of Changes
Description
Pylint will now not emit a
unnecessary-list-index-lookup
warning if the loop / comprehension is iterating overenumerate
called with astart
arg/kwarg.Closes #7682