Skip to content

JS: Fix multiple-comments-on-same-line format#9672

Merged
fisker merged 19 commits intoprettier:masterfrom
fisker:js-comments
Nov 30, 2020
Merged

JS: Fix multiple-comments-on-same-line format#9672
fisker merged 19 commits intoprettier:masterfrom
fisker:js-comments

Conversation

@fisker
Copy link
Copy Markdown
Member

@fisker fisker commented Nov 14, 2020

Fix #9671
Fix #3532
Fix #7724

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker fisker marked this pull request as ready for review November 14, 2020 10:55
@fisker
Copy link
Copy Markdown
Member Author

fisker commented Nov 16, 2020

I just realized that there are similar thing for "RemainingComment"

let indexOfFirstLeadingComment;
, but that logic may not fit this case.

@thorn0
Copy link
Copy Markdown
Member

thorn0 commented Nov 17, 2020

The solution looks good, but I want to test it a bit more.

@fisker
Copy link
Copy Markdown
Member Author

fisker commented Nov 17, 2020

A known issue

Prettier pr-9672
Playground link

--parser babel

Input:

a;
/*1*/;/*2*/
/*3*/
b;

Output:

a; /*2*/
/*1*/ /*3*/
b;

@thorn0
Copy link
Copy Markdown
Member

thorn0 commented Nov 17, 2020

A solution for this known issue might be something like this: if any previous comment for the same precedingNode was detected as "own-line", then the current comment can't be considered "end-of-line" and should be considered "own-line" instead.

@fisker
Copy link
Copy Markdown
Member Author

fisker commented Nov 18, 2020

Stable version "remaining comment" can't handle ; between comments.

Prettier 2.1.2
Playground link

--parser babel

Input:

a;/*1*/ ; /*2*/ b;

a;/*1*/   /*2*/ b;

Output:

a; /*1*/
/*2*/ b;

a;
/*1*/ /*2*/ b;

Comment thread src/main/comments.js Outdated
// Find first comment on the same line
for (let index = commentIndex - 1; index >= 0; index--) {
const previousComment = comments[index];
if (!previousComment) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a check here that comment and previousComment have the same precedingNode. Otherwise, slice might return huge strings when the comments are far from each other.

Comment thread src/main/comments.js Outdated
break;
}
if (previous.precedingNode !== precedingNode) {
continue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
continue;
break;

Comment thread src/main/comments.js Outdated
@thorn0
Copy link
Copy Markdown
Member

thorn0 commented Nov 24, 2020

Prettier pr-9672
Playground link

--parser babel

Input:

`${
a
/*1*//*2*/
/*3*/
}`

a
/*1*//*2*/
/*3*/

Output:

`${
  a /*2*/
  /*1*/
  /*3*/
}`;

a; /*2*/
/*1*/
/*3*/

Comment thread src/main/comments.js
const { locStart, locEnd } = options;
let start = locStart(comment);

if (precedingNode) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition isn't needed. It's okay if precedingNode is falsy. currentCommentPrecedingNode !== precedingNode would still do what we need (we might want to replace !== with != though).

Copy link
Copy Markdown
Member Author

@fisker fisker Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9672 (comment) seems not caused by this part.

a
/*4*//*5*/
/*6*/

The trailing comments part print like this

[
{"type":"line-suffix","contents":{"type":"concat","parts":[{"type":"concat","parts":[{"type":"line","hard":true},{"type":"break-parent"}]},"","/*4*/"]}},
{"type":"concat","parts":[" ","/*5*/"]},
{"type":"line-suffix","contents":{"type":"concat","parts":[{"type":"concat","parts":[{"type":"line","hard":true},{"type":"break-parent"}]},"","/*6*/"]}}
]

Because of line-suffix, /*1*/ print after /*2*/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it's a separate issue, I'll try to fix it

Copy link
Copy Markdown
Member

@thorn0 thorn0 Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three comments should be classified as own-line and printed without line-suffix. I thought it didn't happen because of the if (precedingNode) check. Was I wrong?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were attached as trailing even without this PR, because there is no followingNode, problem is in printTrailingComment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked. These checks indeed don't make a difference. Those comments are classified as own-line with and without these checks.

@fisker fisker merged commit f59eacd into prettier:master Nov 30, 2020
@fisker fisker deleted the js-comments branch November 30, 2020 05:51
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

2 participants