-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[optional ext] Emit redefined-loop-name
for redefinitions of loop variables in body
#5649
[optional ext] Emit redefined-loop-name
for redefinitions of loop variables in body
#5649
Conversation
Pull Request Test Coverage Report for Build 2255071021
💛 - Coveralls |
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'm not very clear on the best way between extending redefined outer name and creating a new checker. We could create a new message without creating a new checker too. I hope for a consensus to be reached in the discussion.
… loop variables in body
ab6e994
to
a80ad9f
Compare
redefined-outer-name
for redefinitions of loop variables in bodyredefined-outer-name
for redefinitions of loop variables in body
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.
Look pretty good already !
Co-authored-by: Pierre Sassoulas <[email protected]>
for more information, see https://pre-commit.ci
@jacobtylerwalls We might need to take a look at the failing |
Outch, if we have that much new warning ourselves I guess it's going to surprise users too. I'm more inclined to create a new optional checker now. |
Yes, I'm inclined toward that also, but I really do think we need to ask whether #1 from the top of the thread should become part of the new checker then. |
Also change message type for inner loops overwriting the outer loop variable from `redefined-outer-name` to `redefined-loop-name`
redefined-outer-name
for redefinitions of loop variables in bodyredefined-loop-name
for redefinitions of loop variables in body
@superbobry do you have thoughts on this approach? This would allow you to disable Also, do you have thoughts on making this optional or default? I will probably disable this in my own projects. |
Updated to make this an optional extension. |
Failing docs check is not relevant, see #6470 |
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.
As I said, I really like the test. Sorry for letting this sit so long, that causes a number of change-requests due to my own aggressive deprecation PRs related to argparse
and mypy
😅 Sorry about that!
Co-authored-by: Daniël van Noord <[email protected]>
for more information, see https://pre-commit.ci
…ls/pylint into redefined-loop-var
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 like the creation of a new checker from the VariableChecker. Some checks definitely need to be disentangled to make maintenance easier.
for more information, see https://pre-commit.ci
Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Daniël van Noord <[email protected]>
for more information, see https://pre-commit.ci
Type of Changes
Description
Closes #5072 -- Adds new message in optional extension
Closes #5608 -- Uncouples the nested loop redefinition case from
redefined-outer-name
so the check can be enabled/disabled appropriatelyUltimately I'm open to the idea of making this into a new message, but then we should get a decision on whether to bring example 1 into the new message also.done!