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.

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

Fix: Handle multiline comments in parseWithComments #12845

Merged
merged 1 commit into from
May 15, 2022

Conversation

ullumullu
Copy link
Contributor

@ullumullu ullumullu commented May 13, 2022

Summary

This problem was found while using prettier with pragmas enabled and I
backtracked it to the parseWithComments function from jest-docblock.
In this context it is especially problematic in cases were the top level
comment is not formatted as a dock lock but a "multi-line comment" to
disable an es-lint rule.

Sample:

/* eslint-disable @typescript-eslint/unbound-method */

Gets transformed to:

/**
 * /* eslint-disable @typescript-eslint/unbound-method
 * /

Especially given the fact that the extract function seems to handle
this case correctly.

Therefore this solution alters the commentStartRe to keep the second * as
optional. This should ensure that all existing use cases work as expected
with proper docblocks but also ensures that an "invalid" multiline comment
works as well.

Links:
https://github.com/ullumullu/jest/blob/main/packages/jest-docblock/src/__tests__/index.test.ts#L29

Test plan

➜  packages git:(fix_multiline) npx jest packages/jest-docblock
 PASS  jest-docblock/src/__tests__/index.test.ts
  docblock
    ✓ extracts valid docblock with line comment (2 ms)
    ✓ extracts valid docblock (1 ms)
    ✓ extracts valid docblock with more comments
    ✓ extracts from invalid docblock
    ✓ extracts from invalid docblock singleline (1 ms)
    ✓ returns extract and parsedocblock (1 ms)
    ✓ parses directives out of a docblock
    ✓ parses multiple of the same directives out of a docblock
    ✓ parses >=3 of the same directives out of a docblock (1 ms)
    ✓ parses directives out of a docblock with comments
    ✓ parses directives out of a docblock with line comments
    ✓ parses multiline directives (1 ms)
    ✓ parses multiline directives even if there are linecomments within the docblock
    ✓ supports slashes in @team directive (1 ms)
    ✓ extracts comments from docblock
    ✓ extracts multiline comments from docblock
    ✓ preserves leading whitespace in multiline comments from docblock (1 ms)
    ✓ removes leading newlines in multiline comments from docblock
    ✓ extracts comments from beginning and end of docblock (1 ms)
    ✓ preserve urls within a pragma's values
    ✓ strip linecomments from pragmas but preserve for comments (1 ms)
    ✓ extract from invalid docblock
    ✓ extract from invalid docblock singleline (8 ms)
    ✓ extracts docblock comments as CRLF when docblock contains CRLF
    ✓ extracts docblock comments as LF when docblock contains LF
    ✓ strips the docblock out of a file that contains a top docblock
    ✓ returns a file unchanged if there is no top docblock to strip
    ✓ prints docblocks with no pragmas as empty string (1 ms)
    ✓ prints docblocks with one pragma on one line
    ✓ prints docblocks with multiple pragmas on multiple lines
    ✓ prints docblocks with multiple of the same pragma
    ✓ prints docblocks with pragmas
    ✓ prints docblocks with comments
    ✓ prints docblocks with comments and no keys
    ✓ prints docblocks with multiline comments (1 ms)
    ✓ prints docblocks that are parseable
    ✓ can augment existing docblocks with comments (1 ms)
    ✓ prints docblocks using CRLF if comments contains CRLF
    ✓ prints docblocks using LF if comments contains LF (1 ms)

Test Suites: 1 passed, 1 total
Tests:       39 passed, 39 total
Snapshots:   0 total
Time:        0.921 s, estimated 1 s
Ran all test suites matching /packages\/jest-docblock/

This problem was found while using prettier with pragmas enabled and I
backtracked it to the `parseWithComments` function from jest-docblock.
In this context it is especially problematic in cases were the top level
comment is not formatted as a dock lock but a "multi-line comment" to
disable an es-lint rule.

/* eslint-disable @typescript-eslint/unbound-method */

Especially given the fact that the `extract` function seems to handle
this case correctly.

Therefore this solution alters the commentStartRe to keep the second * as
optional. This should ensure that all existing use cases work as expected
with proper docblocks but also ensures that an "invalid" multiline comment
works as well.

Links:
https://github.com/ullumullu/jest/blob/main/packages/jest-docblock/src/__tests__/index.test.ts#L29
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit a12b67d into jestjs:main May 15, 2022
@ullumullu ullumullu deleted the fix_multiline branch May 16, 2022 07:22
@ullumullu
Copy link
Contributor Author

Thanks for the review @SimenB!

@SimenB
Copy link
Member

SimenB commented Jun 7, 2022

@github-actions
Copy link

github-actions bot commented Jul 8, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants