Chore: Simplify space-infix-ops #10935
Conversation
eslint-deprecated
bot
added
the
triage
| "*", "/", "%", "+", "-", "<<", ">>", ">>>", "<", "<=", ">", ">=", "in", | ||
| "instanceof", "==", "!=", "===", "!==", "&", "^", "|", "&&", "||", "=", | ||
| "+=", "-=", "*=", "/=", "%=", "<<=", ">>=", ">>>=", "&=", "^=", "|=", | ||
| "?", ":", ",", "**" |
There was a problem hiding this comment.
For what it's worth, I think this list was missing the **= operator, so the rule was failing to report an error for code like this.
lib/rules/space-infix-ops.js
Outdated
| const curr = tokens[i]; | ||
| const next = tokens[i + 1]; | ||
|
|
||
| if (curr.value === "(" || curr.value === ")") { |
There was a problem hiding this comment.
You mentioned that:
there can be a lot of tokens between
leftandright, but only one of them can be an infix operator, the others must be opening and closing parens.
Off the top of my head, I can't think of any cases where this would be wrong, although it's possible that there are some cases that we haven't thought of.
To make sure it's correct, would it be possible to add the expected operator as a parameter to getFirstNonSpacedToken? For example, checkVar could use something like getFirstNonSpacedToken(leftNode, rightNode, "="). Then the implementation of getFirstNonSpacedToken could look something like:
function getFirstNonSpacedToken(left, right, operatorValue) {
const operator = sourceCode.getTokenBetween(
left,
right,
token => token.value === operatorValue
);
const beforeOperator = sourceCode.getTokenBefore(operator);
const afterOperator = sourceCode.getTokenAfter(operator);
// ...check to see if there's a space, as before
}There was a problem hiding this comment.
nice idea, i think it might fix #10922 as well ✊
madbence
force-pushed
the
space-infix-ops-refactor
branch
from
5ed8100
to
7a25462
Compare
lib/rules/space-infix-ops.js
Outdated
| return op; | ||
| } | ||
| function getFirstNonSpacedToken(left, right, op) { | ||
| const operator = sourceCode.getTokensBetween(left, right, token => token.value === op)[0]; |
There was a problem hiding this comment.
i don't think that this'll evaluate to undefined ever, we'll always find the token we're looking for.
lib/rules/space-infix-ops.js
Outdated
| return op; | ||
| } | ||
| function getFirstNonSpacedToken(left, right, op) { | ||
| const operator = sourceCode.getTokensBetween(left, right, token => token.value === op)[0]; |
There was a problem hiding this comment.
Minor nitpick: Could this use getFirstTokenBetween rather than getTokensBetween? (Sorry, my previous example used "getTokenBetween", which
I realized doesn't exist.)
There was a problem hiding this comment.
sure! sorry, i'm not really familiar with the sourceCode API 😺
not-an-aardvark
added
chore
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
What changes did you make? (Give an overview)
space-infix-opsis a bit overcomplicated. there can be a lot of tokens betweenleftandright, but only one of them can be an infix operator, the others must be opening and closing parens. if i'm mistaken, i'd happy to close this PR and open another one that adds those test cases 😸Is there anything you'd like reviewers to focus on?
afaik javascript engines are pretty good at caching loop invariants (so there's no need to save
tokens.length), so i made the loop a bit more readable. i wasn't able to do performance testing (asspace-infix-opsis not part ofeslint:recommended, sonpm run perfis no good), but i'd happy to tweak the implementation if someone can give me some pointer about how to do it.