fix(47923): Delete unused JSDoc quick fix changes formatting on rest of JSDoc comment #48482
Conversation
MQuy
changed the title
MQuy
changed the title
| const line = lines[lineIndex]; | ||
| if (lineIndex === 0) { | ||
| if (line.length > 0) { | ||
| writeSpace(); |
There was a problem hiding this comment.
does this space make sense when the comment is the only thing in the jsdoc? eg
/**
* commentary 1
* commentary 2
*/
var x;It looks like it might not emit the * as expected for the commentary 1 line, or else emit an extra space on the line right after the **.
There was a problem hiding this comment.
it looks the same before and after my change
VS Code
VS Code with my changes
src/compiler/parser.ts
Outdated
| @@ -8183,6 +8195,21 @@ namespace ts { | |||
| else if (comments.length) { | |||
| return comments.join(""); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I'd prefer not to move these down here or add newlines between them.
src/compiler/parser.ts
Outdated
| */ | ||
| // to check tag starting newline without comments like above | ||
| // it needs to actually encounter a newline | ||
| if (states.length === 1 && lookAhead(() => nextTokenJSDoc() !== SyntaxKind.NewLineTrivia)) { |
There was a problem hiding this comment.
nothing ever uses or pops from states; the only read is to check that states.length === 1. It should be a boolean instead, maybe called isFirstLine? That's a guess from reading the code.
| indent += 1; | ||
| break; | ||
| } | ||
| // record the * as a comment | ||
| // falls through | ||
| default: | ||
| /* |
There was a problem hiding this comment.
Can you explain what the parser changes do and why they're needed?
From reading the code, it looks like states is added to know when to call removeTrailingWhitespace, and the change in tryParseTypeExpression now doesn't skip whitespace when a parse fails, but I don't understand the intent of shuffling whitespace parsing around.
There was a problem hiding this comment.
like you mentioned
tryParseTypeExpressiondoesn't skip whitespace when a parse fails
/*
* @param b // <- there is no type expression, it rollback to this position
*
* something here
*/parseTagCommentsonly remove whitespace for these two cases
/*
* @param x
* in the first line
*/
/*
* @param x
still in the first line
*/we can not do it at the end like we are currently doing since it is too late because there is a case with {@link} will reset comments
I don't really get what you mean by I don't understand the intent of shuffling whitespace parsing around, can you explain about it? 😄
| @@ -129,22 +129,22 @@ verify.quickInfos({ | |||
| 3: ["(parameter) param1: string", "first line of param\n\nparam information third line"], | |||
|
|
|||
| g: ["function g(param1: string): void", "This is firstLine\nThis is second Line"], | |||
| 4: ["(parameter) param1: string", " param information first line"], | |||
There was a problem hiding this comment.
I like the old output better; @param is supposed to set a margin so that the space is preserved in param information first line. Preserving the newlines too is OK, depending on how it looks when formatted in VS Code.
This is true of all the changes below.
There was a problem hiding this comment.
current VSCode version
VS Code with my changes
For the the later looks better 😄
|
|
||
| k: ["function k(param1: string): void", "This is firstLine\nThis is second Line"], | ||
| 8: ["(parameter) param1: string", "hello"], | ||
|
|
||
| l: ["function l(param1: string): void", "This is firstLine\nThis is second Line"], | ||
| 9: ["(parameter) param1: string", "first Line text\nblank line that shouldnt be shown when starting this \nsecond time information about the param again"], | ||
| 9: ["(parameter) param1: string", "first Line text\n\n\nblank line that shouldnt be shown when starting this \nsecond time information about the param again"], |
There was a problem hiding this comment.
The intent of the test is to not show the blank lines. I think you're trying to preserve them now, so the text of the test should change to show the changed intent.
Fixes #47923