Skip to content

Fix merged line comments#13438

Merged
thorn0 merged 8 commits intoprettier:nextfrom
thorn0:unmerge-comments
Sep 8, 2022
Merged

Fix merged line comments#13438
thorn0 merged 8 commits intoprettier:nextfrom
thorn0:unmerge-comments

Conversation

@thorn0
Copy link
Copy Markdown
Member

@thorn0 thorn0 commented Sep 6, 2022 •
edited
Loading

Description

Fixes #9959

Checklist

  • 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/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

✨Try the playground for this PR✨

@thorn0 thorn0 linked an issue Sep 6, 2022 that may be closed by this pull request
Single line comments are merged on trailing trivia #9959
Closed
fisker
fisker reviewed Sep 7, 2022
View reviewed changes
Comment thread src/main/comments.js Outdated
if (
previousCommentDocType === DOC_TYPE_LINE_SUFFIX ||
(previousCommentDocType === DOC_TYPE_ARRAY &&
getDocType(previousComment.doc[0]) === DOC_TYPE_LINE_SUFFIX)
Copy link
Copy Markdown
Member

@fisker fisker Sep 7, 2022

Choose a reason for hiding this comment

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

Should we use findInDoc? The printed doc may change in future.

Copy link
Copy Markdown
Member Author

@thorn0 thorn0 Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The tests should save us. findInDoc is recursive. Seems like too much overhead.

As an alternative solution, what if we change the return type of this function to { doc, isBlock, hasLineSuffix }?

Copy link
Copy Markdown
Member

@fisker fisker Sep 7, 2022

Choose a reason for hiding this comment

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

Much better!

@fisker
Copy link
Copy Markdown
Member

fisker commented Sep 7, 2022

Since you already got previous comment, may a simple check here https://github.com/thorn0/prettier/blob/ed63ad5928f5ba1dd60e5e6c0fda5906584b3989/src/main/comments.js#L511 will fix #12653?

@thorn0
Copy link
Copy Markdown
Member Author

thorn0 commented Sep 7, 2022

Since you already got previous comment, may a simple check here https://github.com/thorn0/prettier/blob/ed63ad5928f5ba1dd60e5e6c0fda5906584b3989/src/main/comments.js#L511 will fix #12653?

#12653 is about leading comments too, not only trailing ones. Let's fix it separately. BTW, we're going to need a new printer method for that check because it's a JS-specific thing.

@github-actions github-actions Bot mentioned this pull request Sep 7, 2022
Closed
@thorn0 thorn0 merged commit d2a0c77 into prettier:next Sep 8, 2022
@thorn0 thorn0 deleted the unmerge-comments branch September 8, 2022 06:45
This was referenced Sep 15, 2022
Closed
Closed
@thorn0 thorn0 linked an issue Oct 21, 2022 that may be closed by this pull request
Print comment type includes as comments #3493
Closed
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 1, 2024
@thorn0 @medikoo
Fix merged line comments (prettier#13438)
5ad2cb5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fisker fisker fisker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Single line comments are merged on trailing trivia Print comment type includes as comments

2 participants

@thorn0 @fisker