Fix: fix the fixer of lone comma with comments (fixes #10632) #11154
Conversation
eslint-deprecated
bot
added
the
triage
There was a problem hiding this comment.
Left a suggestion about further improving the line break matcher.
The test case looks good in general, but I'm not following what changed. Could you please walk me through what the test case's original output was, and how your change in the rule code helped fix it?
(Part of the issue is that usually, bug fixes are converting a valid case into an invalid case, or vice versa, and RuleTester is almost self-documenting in those cases; in this case, we're taking an invalid case with a bad autofix and turning it into an invalid case with better autofix. RuleTester does not document that quite as well.)
lib/rules/comma-style.js
Outdated
| @@ -85,7 +85,7 @@ module.exports = { | |||
| function getReplacedText(styleType, text) { | |||
| switch (styleType) { | |||
| case "between": | |||
| return `,${text.replace("\n", "")}`; | |||
| return `,${text.replace(/\r?\n/, "")}`; | |||
There was a problem hiding this comment.
Might be good to look in ast-utils; I think we have a regular expression stored in there which handles all of the JS line break types, including \u2028 and \u2029.
There was a problem hiding this comment.
Thanks! And I have updated. You can check the latest changes.
|
@platinumazure I think you can use the ESLint online demo to see how ESLint behaves currently. So here I will talk the changed output and how it works. Consider the input below: [
one // line
, /* block
comment */
]And the changed output will be: [
one, // line
/* block
comment */
]In the case above, there is a block comment after comma token, so in my changes you can see it gets the commemts after the comma token. Then, we check the comment and make sure it's a block comment and the comment keeps with comma token on the same line. (If it doesn't match this check, just does as current.) The case above matches the check. On the other hand, if there is a real token after the comma, the auto fixer works well. So, at this case, we treat the block comment as a token (Yeah, it's not a real token because comment isn't a token). Next, we can use the (I'm not an English speaker and sorry for my poor English.) |
|
For what it's worth @g-plane, your English is great! And I apologize if it seemed I had difficulty understanding due to how you were writing- in reality, I was just having difficulty understanding the technical change. |
platinumazure
added
bug
|
@g-plane Thanks for the link, it helps! I assume there is an existing test case that shows the previous logic (working correctly, without a block comment after the comma)? |
|
@platinumazure That's right. |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix
What changes did you make? (Give an overview)
Fixes #10632
Is there anything you'd like reviewers to focus on?
When I tried reproducing the issue, I found that if the source code uses CRLF as linebreak, this issue may not occur, because inside the function
getReplacedText, thebetweenbranch only replaces LF with empty string. So in this PR, I fix it too.