Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix merged line comments #13438

Merged
merged 8 commits into from Sep 8, 2022
Merged

Fix merged line comments #13438

merged 8 commits into from Sep 8, 2022

Conversation

thorn0
Copy link
Member

@thorn0 thorn0 commented Sep 6, 2022 •
edited

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
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
Member

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
Member Author

@thorn0 thorn0 Sep 7, 2022 •
edited

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
Member

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
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
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