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: Check division operator in astUtils.canTokensBeAdjacent #12879
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1357,17 +1357,43 @@ module.exports = { | |
* next to each other, behavior is undefined (although it should return `true` in most cases). | ||
*/ | ||
canTokensBeAdjacent(leftValue, rightValue) { | ||
const espreeOptions = { ecmaVersion: 2020, comment: true, range: true }; | ||
|
||
let leftToken; | ||
|
||
if (typeof leftValue === "string") { | ||
const leftTokens = espree.tokenize(leftValue, { ecmaVersion: 2015 }); | ||
const tokens = espree.tokenize(leftValue, espreeOptions); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind adding a test for when a For reference, we mutate the text input to make the shebang a regular JS line comment before we parse in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, that would crash! Maybe we could already add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
astUtils.canTokensBeAdjacent(someToken, "/* comment */"); Then, it might also make sense to support empty code as well, and maybe even to account for spaces at the start/end of the code (e.g., Maybe we could do that as well in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds good to me! I guess another option (or something we could do additionally) is to short circuit it and add an early return of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, interesting! Since this is already an improvement over the existing code, I would be fine with splitting these bug fixes over multiple PRs to keep them manageable. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe I didn't understand, but I think it isn't possible to check if the token is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see, I missed that it could be a string or a token. Yes, if the left value is a string that won't work. I think the approach you suggested sounds good 👍 |
||
const comments = tokens.comments; | ||
|
||
leftToken = tokens[tokens.length - 1]; | ||
if (comments.length) { | ||
const lastComment = comments[comments.length - 1]; | ||
|
||
leftToken = leftTokens[leftTokens.length - 1]; | ||
if (lastComment.range[0] > leftToken.range[0]) { | ||
leftToken = lastComment; | ||
} | ||
} | ||
} else { | ||
leftToken = leftValue; | ||
} | ||
|
||
const rightToken = typeof rightValue === "string" ? espree.tokenize(rightValue, { ecmaVersion: 2015 })[0] : rightValue; | ||
let rightToken; | ||
|
||
if (typeof rightValue === "string") { | ||
const tokens = espree.tokenize(rightValue, espreeOptions); | ||
const comments = tokens.comments; | ||
|
||
rightToken = tokens[0]; | ||
if (comments.length) { | ||
const firstComment = comments[0]; | ||
|
||
if (firstComment.range[0] < rightToken.range[0]) { | ||
rightToken = firstComment; | ||
} | ||
} | ||
} else { | ||
rightToken = rightValue; | ||
} | ||
|
||
if (leftToken.type === "Punctuator" || rightToken.type === "Punctuator") { | ||
if (leftToken.type === "Punctuator" && rightToken.type === "Punctuator") { | ||
|
@@ -1379,6 +1405,9 @@ module.exports = { | |
MINUS_TOKENS.has(leftToken.value) && MINUS_TOKENS.has(rightToken.value) | ||
); | ||
} | ||
if (leftToken.type === "Punctuator" && leftToken.value === "/") { | ||
return !["Block", "Line", "RegularExpression"].includes(rightToken.type); | ||
} | ||
return true; | ||
} | ||
|
||
|
@@ -1393,6 +1422,10 @@ module.exports = { | |
return true; | ||
} | ||
|
||
if (leftToken.type === "Block" || rightToken.type === "Block" || rightToken.type === "Line") { | ||
return true; | ||
} | ||
|
||
return false; | ||
}, | ||
|
||
|
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-blocker for this PR, but it could be nice to add the ability to specify “latest” somehow, so that this never gets out of date.
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.
That would be great! But, I'm not sure if there is a way to do that at the moment? Does
espree
support something like "latest" or provides a list of available versions?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.
v/_ \ I don't think
espree
has such a thing.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 doesn’t, no! Definitely not a blocker for this - just an idea I had and wanted to write down for posterity 😄
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 list of all versions could be also useful for the online demo, it's hard-coded there and has to be updated manually.
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.
PR/proposal here: eslint/espree#430