Skip to content

Fix bug: keep comments unless --removeComments #7213

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

Closed
wants to merge 1 commit into from

Conversation

staltz
Copy link

@staltz staltz commented Feb 24, 2016

Fixes #2546.

This is a straightforward fix for bug #2546. Keeps most or all TS comments
in the output, unless the option --removeComments is enabled.

(CLA signed, DocuSign Envelope ID: 2F26CC42-D7EE-48C6-B680-A6C0E3F7E58F)

This is a straightforward fix for bug microsoft#2546. Keeps most or all TS comments
in the output, unless the option --removeComments is enabled.
@msftclas
Copy link

Hi @staltz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

// Ambient class
// Ambient enum
// Ambient enum with integer literal initializer
// Ambient enum members are always exported with or without export keyword
Copy link
Member

Choose a reason for hiding this comment

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

This output is undesirable because it's extremely misleading. There's a reason we don't emit every comment, and this is the typical example of why.

Copy link
Author

Choose a reason for hiding this comment

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

But issue #2546 is precisely about keeping these comments. And it said PRs are welcomed. Can we define what are precisely the conditions to hide or show comments? Should we keep all comments of the type /** */ or also // (the original reporter in #2546 wanted the latter too)?

Copy link
Member

Choose a reason for hiding this comment

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

Can we define what are precisely the conditions to hide or show comments?

Honestly, that's the hard part (if we had a good rule, we would have done it already). Clearly leading comments for type declarations shouldn't be present in the emit as they look like misleading nonsense, but comments between an opening brace of a statement block and another expression statement should be present.

Figuring out where it makes sense to emit the comments and where it doesn't is the hard part. Simply emitting every comment isn't the right fix.

@DanielRosenwasser
Copy link
Member

Thanks for sending this out @staltz, but since this isn't the correct approach, I'm going to close this PR. Feel free to follow up with a new one though.

@staltz
Copy link
Author

staltz commented Feb 26, 2016

I'm not going to submit a new PR until there is a clear notion of what #2546 actually means and what is the acceptance criteria. It serves us (RxJS project) a real purpose: being able to keep JSDoc-style comments (/** */) for things like interfaces et al.

@benlesh
Copy link

benlesh commented Feb 26, 2016

Is it really a transpiler's business to decide which comments have value? Transpilers can't read. Seems strange. IMO, this should be "all in" or "all out" barring comments that take some standard convention that signifies something you don't want removed to contain, say, a license or copyright. There is prior art for that.

@grlscz
Copy link

grlscz commented Apr 4, 2016

May we have a flag to keep ALL comments, if we really want it?

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript removing comments where it shouldn't
6 participants