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

Support nestled JSDoc comments #13445

Merged
merged 8 commits into from Sep 13, 2022
Merged

Support nestled JSDoc comments #13445

merged 8 commits into from Sep 13, 2022

Conversation

thorn0
Copy link
Member

@thorn0 thorn0 commented Sep 8, 2022 •
edited

Description

Fixes #12653

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✨

fisker
fisker reviewed Sep 8, 2022
View reviewed changes
tests/format/js/comments/__snapshots__/jsfmt.spec.js.snap Outdated
Comment on lines 3155 to 3156
/** Trailing nestled comment 1
*/ /** Trailing nestled comment 2 */
Copy link
Member

Choose a reason for hiding this comment

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

Should they stick together?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on whether we need to support this for single-line comments.

@thorn0
Copy link
Member Author

thorn0 commented Sep 8, 2022

Oops! Forgot to test dangling comments:

Prettier pr-13445
Playground link

--parser babel

Input:

{{{{{{{
o={
  /**
   * nestled
   *//**
   * nestled
   */
  
}
}}}}}}}

Output:

{
  {
    {
      {
        {
          {
            {
              o = {
                /**
                 * nestled
                 */
/**
                 * nestled
                 */
              };
            }
          }
        }
      }
    }
  }
}

@fisker
Copy link
Member

fisker commented Sep 8, 2022

Maybe easier to merge them into one comment after parse?

@thorn0
Copy link
Member Author

thorn0 commented Sep 8, 2022

The code for printing dangling comments needs to be refined anyway. Now it's very basic. It prints every comment on a new line and doesn't preserve blank lines between comments. Instead its logic should be similar to how trailing comments are printed. But it's something for a separate PR. As for this PR, what if we just skip this feature for dangling comments for now? I'll just add a check that dangling comments can't be considered nestled.

@thorn0 thorn0 requested a review from fisker September 8, 2022 13:37
@fisker
Copy link
Member

fisker commented Sep 8, 2022

Any comment on this idea? #13445

@thorn0
Copy link
Member Author

thorn0 commented Sep 8, 2022

IDK. Merging would work too, but it's difficult to say if it's going to make things simpler.

@fisker
Copy link
Member

fisker commented Sep 8, 2022

it's difficult to say if it's going to make things simpler.

Shouldn't it work without any change to the print part?

@thorn0
Copy link
Member Author

thorn0 commented Sep 8, 2022

For the print part - yes. I thought merging itself might be a bit tricky, but I may be wrong.

@fisker
Copy link
Member

fisker commented Sep 8, 2022

Aren't they looks like one comment? Would like to try?

@thorn0
Copy link
Member Author

thorn0 commented Sep 8, 2022

Yes, I'm going to try that approach.

@thorn0 thorn0 marked this pull request as draft September 9, 2022 14:07
fisker
fisker reviewed Sep 10, 2022
View reviewed changes
src/language-js/parse/postprocess/index.js Outdated
isIndentableBlockComment(comment) &&
isBlockComment(nextComment) &&
isIndentableBlockComment(nextComment) &&
locEnd(comment) === locStart(nextComment)
Copy link
Member

Choose a reason for hiding this comment

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

loc compare should be faster than isIndentableBlockComment check, let's move this condition up

@fisker
Copy link
Member

fisker commented Sep 10, 2022 •
edited

Maybe we should extract isIndentableBlockComment

Size Change: +605 kB (+6%) 🔍

Total Size: 11.5 MB

Filename Size Change
./dist/esm/parser-babel.mjs 368 kB +61.5 kB (+20%) 🚨
./dist/esm/parser-espree.mjs 212 kB +61.8 kB (+41%) 🚨
./dist/esm/parser-flow.mjs 1.11 MB +63.2 kB (+6%) 🔍
./dist/esm/parser-meriyah.mjs 221 kB +61.8 kB (+39%) 🚨
./dist/esm/parser-typescript.mjs 1.49 MB +54.4 kB (+4%)
./dist/parser-babel.js 369 kB +61.5 kB (+20%) 🚨
./dist/parser-espree.js 213 kB +61.8 kB (+41%) 🚨
./dist/parser-flow.js 1.11 MB +63.2 kB (+6%) 🔍
./dist/parser-meriyah.js 221 kB +61.8 kB (+39%) 🚨
./dist/parser-typescript.js 1.49 MB +54.4 kB (+4%)

@thorn0 thorn0 marked this pull request as ready for review September 10, 2022 20:12
@thorn0
Copy link
Member Author

thorn0 commented Sep 10, 2022 •
edited

BTW, I agree with this opinion on default exports https://mobile.twitter.com/stefanjudis/status/1566047068510093315 :)

fisker
fisker reviewed Sep 13, 2022
View reviewed changes
src/language-js/parse/postprocess/index.js Outdated
...comment,
value: comment.value + "*//*" + followingComment.value,
range: [locStart(comment), locEnd(followingComment)],
};
Copy link
Member

Choose a reason for hiding this comment

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

We already mutate the array, let's mutate the comment directly?

fisker
fisker approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Good job!

src/language-js/parse/postprocess/index.js Outdated Show resolved Hide resolved
@thorn0 @fisker
Update src/language-js/parse/postprocess/index.js
f7fd5c7 
Co-authored-by: fisker Cheung <lionkay@gmail.com>
thorn0
thorn0 commented Sep 13, 2022
View reviewed changes
.eslintrc.cjs
@@ -329,7 +330,6 @@ module.exports = {
functions: ["hasComment", "getComments"],
},
"src/language-js/pragma.js",
"src/language-js/parse/postprocess/*.js",
Copy link
Member Author

@thorn0 thorn0 Sep 13, 2022 •
edited

Choose a reason for hiding this comment

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

This didn't work because the rule doesn't resolve globs.

@thorn0 thorn0 merged commit 1f46acc into prettier:next Sep 13, 2022
@thorn0 thorn0 deleted the jsdoc-neslted branch September 13, 2022 12:24
@thorn0 thorn0 mentioned this pull request Sep 26, 2022
Closed
@thorn0 thorn0 mentioned this pull request Oct 7, 2022
Closed
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 1, 2024
@thorn0 @medikoo
Support nestled JSDoc comments (prettier#13445)
f788253
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.

None yet
2 participants
@thorn0 @fisker