-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Avoid naming collision with default array element variable in autofix for no-for-loop
rule
#749
Conversation
165ecad
to
ae3e0bb
Compare
@fisker I added one more test case. |
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.
Please fix tests
…tofix for `no-for-loop` rule
231eed9
to
a1c5b30
Compare
@fisker thanks a lot for adding additional test cases. I fixed the failure. |
rules/no-for-loop.js
Outdated
@@ -335,7 +343,7 @@ const create = context => { | |||
const shouldGenerateIndex = isIndexVariableUsedElsewhereInTheLoopBody(indexVariable, bodyScope, arrayIdentifierName); | |||
|
|||
const index = indexIdentifierName; | |||
const element = elementIdentifierName || defaultElementName; | |||
const element = elementIdentifierName || avoidCapture(defaultElementName, getChildScopesRecursive(bodyScope), context.parserOptions.ecmaVersion); |
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 think this is not right.
for (let i = 0; i < arr.length; i += 1) {
console.log(arr[1]);
function foo(element) {
console.log(element);
}
}
It should allow use variable element
.
this foo
function scope should not included, because it didn't use arr[1]
, this is why avoidCapture
didn't recursive automaticlly.
We need collect scopes from references.
BTW: don't squash commits, this makes review harder.
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.
A few questions for you.
- By
arr[1]
, do you meanarr[i]
? The rule doesn't report a violation witharr[1]
. - In your example, I see that we technically can use
element
as our new array variable name, but wouldn't that be unnecessarily confusing? The no-shadow rule suggests this is bad practice.
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.
- Yes,
arr[1]
, sorry. - Yes
no-shadow
suggest no variable shadowing, it's default tofunction
, do you useall
option? My example is about function scope, but there are other scopes.
This example
for (let i = 0; i < errors.length; i++) {
console.log(errors[i]);
try {
doSomethingThrowError();
} catch(error) {}
}
After #745 we will fix it to
for (const error_ of errors) {
console.log(error_);
try {
doSomethingThrowError();
} catch(error) {}
}
I don't want use error_
, I can use error
.
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.
Anyway, this is not very important, let's keep it this way now.
I'm going to check this PR again.
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.
Thanks for the explanation. I will follow-up with your recommendation if I have a chance.
Thanks for fixing this. |
Fixes #747. Will also help prepare us for #745.