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: no-plusplus allow comma operands in for afterthought (fixes #13005) #13024
Conversation
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! Thanks 👍
}] | ||
}, | ||
{ | ||
code: "for (;; i++, f(--j));", |
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.
non-blocking.Just questions for understanding the proper behavior of this option.
It seems the option{allowForLoopAfterthoughts: true}
changed to allow update expression
s in sequence expression
which has exactly one depth from for-loop update
.
so the case like f(--j)
is still warned.
And so, the below cases(nested sequence expression) are intended in the same manner?
for (;; (++a, (++b, ++c) ) {} // it's warned in the PR version. no-plusplus for `b` and `c`
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.
Didn't think of that. It's done now, thanks!
* @param {ASTNode} node The node to check. | ||
* @returns {boolean} `true` if the node is a for loop afterthought. | ||
*/ | ||
function isForLoopAfterthought(node) { |
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 @yeonjuan mentioned, it appears that this check only looks up one level, which could mean some patterns will still be flagged as incorrect.
for (;; (++a, (++b, ++c) ) {} // it's warned in the PR version. no-plusplus for `b` and `c`
It may be an unlikely case, but for the sake of completeness, it probably makes sense to continue looking up the AST if the parent is a SequenceExpression
.
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.
It's done now! It makes sense to me, too.
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 Thanks!
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.
Awesome job, thanks!
LGTM, thanks! |
…nt#13005) (eslint#13024) * Fix: no-plusplus allow comma operands in for afterthought (fixes eslint#13005) * Allow deep sequence operands
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Bug fix
fixes #13005
What changes did you make? (Give an overview)
allowForLoopAfterthoughts
in theno-plusplus
rule will now additionally allow++
and--
in sequence expressions:Also fixed false negatives when the node isn't the
update
node:Is there anything you'd like reviewers to focus on?