-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Fix: make no-unused-vars
ignore for-in loops (fixes #2342)
#6126
Conversation
LGTM |
By analyzing the blame information on this pull request, we identified @vitorbal, @nzakas and @mysticatea to be potential reviewers |
I assume phantom issue is known problem |
Yes, phantom issue has been screwing our builds for a while now... |
@markelog I think the agreed-upon change was to ignore only |
Is there ticket about this? Downloading phantom binary is known bottleneck and that's syntactic environment, since alternative would be to actually make ci run tests in real browsers, i would propose to do that instead. mm, i was under impression Otherwise we would need to inspect body of the loop, which would introduce it seems unnecessary complexity. I'm not sure what kind of disadvantages we would try to avoid by doing that, i could see though breaking a use-case of object getters on which that loop could be iterated upon if we would be specific with |
Agree with @alberto - the resolution of #2342 is to explicitly exempt for-in when it's body is just @markelog can you update this PR accordingly? Also, can you update the commit message to begin with "Update" instead of "Fix"? (The issue is labeled as an enhancement, so we use " Update" for that.) |
ok-ok, so we targeting only loops with one statement which should be |
Only for-in loops with |
Sorry to keep asking about this, but i want to be clear about it - only one statement with for (var key in obj) {
i++;
return false;
} Is violation or not? Also your last comment was #2342 (comment) yeah, but then there was #2342 (comment) which includes
Those statements confuse a bit |
Just |
ok, thank you for clarification |
Done, thank you |
LGTM |
Thanks for the pull request, @markelog! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
LGTM |
@@ -99,7 +99,6 @@ module.exports = { | |||
* Determines if a given variable is being exported from a module. | |||
* @param {Variable} variable - EScope variable object. | |||
* @returns {boolean} True if the variable is exported, false if not. | |||
* @private |
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.
Why did you remove @private
everywhere?
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.
There is a lot of rules that do use this keyword and there is a lot of rules that don't, some of the helpers of this particular rule also do and do not.
So it seems we have to choose with or without it, since in my view it is already obvious they are private, i choice the latter.
Trying to make it consistent, what way seems better to you?
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.
So what should i do?
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'd prefer to have @private
, as it will prevent JSDoc from generating docs for them if we ever get that working 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.
ok-ok, also, i think it would the right move to do this everywhere else, how about creating a ticket about this?
Will make a fix for other remarks after clarification, so it could be done in one move |
Done, sorry for the delay |
@markelog It looks like there's a still merge conflict in this PR. Could you rebase please? |
LGTM |
Donzo, there is a lot of interesting code in there. |
target = target.body; | ||
} | ||
|
||
return target.type === "ReturnStatement"; |
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 guess this may cause null dereference error at empty block 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.
mm, could you elaborate with some code?
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.
(function(obj) { var name; for ( name in obj ) { } })({});
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.
Fixed thanks, btw, was about to cc you here and there you are :)
LGTM |
I'd like to see a test for the following code:
|
You mean for |
I'm seeing that this is checking whether the parent node of an identifier is a ForInStatement or not. |
no-unused-vars
ignore for...(in | of) loops (fixes #2342)no-unused-vars
ignore for-in loops (fixes #2342)
Applied only for `for-in` loops with only one `return` statement. Also clean jsdocs for internal helpers
LGTM |
@mysticatea done |
Thank you very much! |
Okay, so it seems i have addressed all concerns, so if no one objects, i will land this today |
@markelog why was the commit message changed from "Fix" to "Update" here? Note that as soon as it becomes "Update", a TSC member needs to merge the code (because it causes a minor semver bump). |
Applied only for `for-in` loops with only one `return` statement. Also clean jsdocs for internal helpers
Applied only for `for-in` loops with only one `return` statement. Also clean jsdocs for internal helpers
Applied only for `for-in` loops with only one `return` statement. Also clean jsdocs for internal helpers
FYI: We changed the commit message back to "Fix" as it seems like that is what is appropriate given the semver policy. |
Thanks @markelog for keeping up! |
Also clean jsdocs for internal helpers