Fix: multiline-comment-style rule add extra space after * (fixes #12785) #12823

karthikkp · 2020-01-22T21:20:34Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] 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
[ ] Other, please explain:

fixes #12785

What changes did you make? (Give an overview)

Added the extra white space which was removed here https://github.com/eslint/eslint/pull/12316/files#diff-e8cce2004dd5e0386021b0e80370c8ebL190

Is there anything you'd like reviewers to focus on?

No

…#12785)

kaicataldo

Thanks for working on this! This looks like a regression I introduced in #12316. As you can see from the tests, the current behavior is intentional. If the original comment didn't have a leading offset space, it currently tries to preserve that formatting when it autofixes.

I think the correct fix here would be to check if any line starts with a / character and, if there isn't leading whitespace being added, set the offset to a single space character to all lines. This would allow the rule to function as it does now for the majority of cases and handle this edge case gracefully.

kaicataldo · 2020-01-22T22:08:38Z

Additionally, we require tests for all changes. Feel free to start with the case reported in the issue!

karthikkp · 2020-01-22T22:13:22Z

Thanks for working on this! This looks like a regression I introduced in #12316. As you can see from the tests, the current behavior is intentional. If the original comment didn't have a leading offset space, it currently tries to preserve that formatting when it autofixes.

I think the correct fix here would be to check if any line starts with a / character and, if there isn't leading whitespace being added, set the offset to a single space character to all lines. This would allow the rule to function as it does now for the majority of cases and handle this edge case gracefully.

Actually the problem is with the regular expression here https://github.com/eslint/eslint/pull/12316/files#diff-e8cce2004dd5e0386021b0e80370c8ebR316 and not the diff in the description. If we split the regular expression into two parts, one for the prefix before * and one after, I can get it to work. Do you want me to do that instead of "If the original comment didn't have a leading offset space, it currently tries to preserve that formatting". That way it code will be generic and we may not have to just check that one edge case specifically.

Mean time I will try this and let you know

kaicataldo · 2020-01-22T22:22:33Z

To be clear, the behavior before the PR mentioned in the description was very different how it is now.

Can you share an example of what you're thinking? I'm not sure I'm following. I think it's working correctly for other cases besides this one edge case, and we should keep the current behavior the same (which should be easy to verify since this rule is tested fairly well).

karthikkp · 2020-01-22T22:25:16Z

So what I am saying is, in the diff pasted, this problem occurs when the preceeding line also does not have * and initial offset hence is an empty string. So I can just add a check to see if the initial offset is an empty string and if it is just set initialoffset to a string with a space.

kaicataldo · 2020-01-22T22:28:30Z

@karthikkp If I'm understanding correctly, that would cause the tests I linked above to fail. The rule tries to preserve the leading whitespace (or lack thereof) when it's converting the comment, so it's sometimes correct for the initial offset to be an empty string. I think this is the correct behavior (and was intentional when I made that change). Are there any other cases where malformed code can occur other than when there's no leading whitespace and the first character of a comment line is a / character?

karthikkp · 2020-01-22T22:42:29Z

Oh okay got it. Then I think handling the edge case alone will be the way to go. Will do that

kaicataldo

Thanks for a working on this! Do you mind updating the PR title? I believe it's currently failing the commit message check because it is too long.

kaicataldo · 2020-01-23T18:17:09Z

tests/lib/rules/multiline-comment-style.js

@@ -1379,6 +1379,25 @@ ${"                   "}
                { messageId: "missingStar", line: 4 },
                { messageId: "endNewline", line: 4 }
            ]
+        },


Can we make this test suite a bit more robust by adding other kinds of formatted comments being formatted to starred-block?

Some examples:

////This comment is in //`separate-lines` format. // // This comment is in // `separate-lines` format. /* * // a line comment *some.code(); */ /* // a line comment * some.code(); */

The first two are not converted to starred block comments in the current version. So the fix that I am working should not change that behavior right?

Oh, interesting. Yes, that makes sense to me. Thanks for adding these tests.

yeonjuan

Thanks for working on this! I left a question about the result of fixing.

lib/rules/multiline-comment-style.js

…t white space after it.

yeonjuan

Thanks for the changes! I left a minor suggestion.

lib/rules/multiline-comment-style.js

kaicataldo

LGTM, thank you!

kaicataldo · 2020-01-27T21:42:51Z

tests/lib/rules/multiline-comment-style.js

@@ -1379,6 +1379,25 @@ ${"                   "}
                { messageId: "missingStar", line: 4 },
                { messageId: "endNewline", line: 4 }
            ]
+        },


Oh, interesting. Yes, that makes sense to me. Thanks for adding these tests.

karthikkp · 2020-01-27T21:59:53Z

@yeonjuan The changes you had requested have been made 👍

yeonjuan

LGTM, thanks!

kaicataldo · 2020-01-27T23:45:03Z

Thanks for contributing to ESLint!

…nt#12785) (eslint#12823) * Fix: multiline-comment-style rule missed an extra space after * (fixes eslint#12785) * logic changed to handled only the edge case * test case added for the edge case * checks full offset and apply space after * only if there already isn't white space after it. * addeed new test case

Fix: multiline-comment-style rule missed an extra space after * (fixes …

2f6afb7

…#12785)

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 22, 2020

kaicataldo suggested changes Jan 22, 2020

View reviewed changes

karthikkp added 2 commits January 23, 2020 20:15

logic changed to handled only the edge case

fc2ba59

test case added for the edge case

1ef1354

kaicataldo suggested changes Jan 23, 2020

View reviewed changes

yeonjuan suggested changes Jan 23, 2020

View reviewed changes

lib/rules/multiline-comment-style.js Outdated Show resolved Hide resolved

checks full offset and apply space after * only if there already isn'…

add0e21

…t white space after it.

yeonjuan reviewed Jan 26, 2020

View reviewed changes

lib/rules/multiline-comment-style.js Outdated Show resolved Hide resolved

addeed new test case

73c99ec

karthikkp requested review from yeonjuan and kaicataldo January 27, 2020 11:54

karthikkp changed the title ~~Fix: multiline-comment-style rule missed an extra space after * (fixes #12785)~~ Fix: multiline-comment-style rule add extra space after * (fixes #12785) Jan 27, 2020

kaicataldo approved these changes Jan 27, 2020

View reviewed changes

yeonjuan approved these changes Jan 27, 2020

View reviewed changes

kaicataldo merged commit 533c114 into eslint:master Jan 27, 2020

eslint-deprecated bot locked and limited conversation to collaborators Jul 27, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: multiline-comment-style rule add extra space after * (fixes #12785) #12823

Fix: multiline-comment-style rule add extra space after * (fixes #12785) #12823

karthikkp commented Jan 22, 2020

kaicataldo left a comment •

edited

kaicataldo commented Jan 22, 2020

karthikkp commented Jan 22, 2020

kaicataldo commented Jan 22, 2020

karthikkp commented Jan 22, 2020

kaicataldo commented Jan 22, 2020 •

edited

karthikkp commented Jan 22, 2020

kaicataldo left a comment

kaicataldo Jan 23, 2020

karthikkp Jan 25, 2020

kaicataldo Jan 27, 2020

yeonjuan left a comment

yeonjuan left a comment

kaicataldo left a comment

kaicataldo Jan 27, 2020

karthikkp commented Jan 27, 2020

yeonjuan left a comment

kaicataldo commented Jan 27, 2020

Navigation Menu

Fix: multiline-comment-style rule add extra space after * (fixes #12785) #12823

Fix: multiline-comment-style rule add extra space after * (fixes #12785) #12823

Conversation

karthikkp commented Jan 22, 2020

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

kaicataldo left a comment • edited

Choose a reason for hiding this comment

kaicataldo commented Jan 22, 2020

karthikkp commented Jan 22, 2020

kaicataldo commented Jan 22, 2020

karthikkp commented Jan 22, 2020

kaicataldo commented Jan 22, 2020 • edited

karthikkp commented Jan 22, 2020

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Jan 23, 2020

Choose a reason for hiding this comment

karthikkp Jan 25, 2020

Choose a reason for hiding this comment

kaicataldo Jan 27, 2020

Choose a reason for hiding this comment

yeonjuan left a comment

Choose a reason for hiding this comment

yeonjuan left a comment

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo Jan 27, 2020

Choose a reason for hiding this comment

karthikkp commented Jan 27, 2020

yeonjuan left a comment

Choose a reason for hiding this comment

kaicataldo commented Jan 27, 2020

kaicataldo left a comment •

edited

kaicataldo commented Jan 22, 2020 •

edited