Skip to content

allow consecutive newlines in jsdoc tag comments #38036

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

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

michens
Copy link
Contributor

@michens michens commented Apr 18, 2020

Fixes #35511

The parser for JSDoc tag comments currently skips successive NewLineTrivia tokens.
In files using CRLF line endings, this leads to only \r being retained in the node, which can cause issues for tooling.
This PR removes that restriction, such that all NewLineTrivia tokens are persisted in the node.

@sandersn sandersn self-assigned this May 5, 2020
@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label May 5, 2020
@sandersn sandersn requested review from sandersn and andrewbranch May 5, 2020 18:24
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This is probably a good change, but it's not the correct fix for #35511 — the jsdoc scanner should treat \r\n as a single newline, but doesn't. #39351 addresses that.

However, skipping successive newlines doesn't make too much sense to me since JSDoc is generally pretty sensitive to whitespace, so I think it's a good idea get rid of the rule.

I approved the PR, but can you please merge from master after #39351 is in? I can do it when I have the chance if you don't want to switch back to this PR.

@michens
Copy link
Contributor Author

michens commented Jul 2, 2020

This has been merged with master after your changes.

@andrewbranch
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 2, 2020

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 1cbf239. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/78496/artifacts?artifactName=tgz&fileId=C928C1027B0D36A39755BE621DF82E17C9EC9855604949D4070B3B55674587C202&fileName=/typescript-4.0.0-insiders.20200702.tgz"
    }
}

and then running npm install.

@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, something went wrong when looking for the build artifact. (You can check the log here).

@sandersn sandersn merged commit 2d09da6 into microsoft:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

JSDocTag#comment has CRLF newlines as CR
4 participants