Skip to content

fix(30003): formatter deletes comment after trailing comma (again) #49168

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
May 27, 2022

Conversation

TRCYX
Copy link
Contributor

@TRCYX TRCYX commented May 18, 2022

Fixes #30003 again.

As I mentioned in #30003 (comment), the original fix, PR #36674, only checks for cases where the trailing comma is on the same line as (, failing on cases like

fu(
    5, /*afsd*/);

The problem originates from that when the NoSpaceBeforeCloseParen rule was applied, the relevant tokens were 5 and ), making the rule treat everything in between as whitespace to be deleted. This happened because the comma was only advance'd over, not consumed, so not recorded:

if (listEndToken !== SyntaxKind.Unknown && formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) {
let tokenInfo: TokenInfo | undefined = formattingScanner.readTokenInfo(parent);
if (tokenInfo.token.kind === SyntaxKind.CommaToken && isCallLikeExpression(parent)) {
const commaTokenLine = sourceFile.getLineAndCharacterOfPosition(tokenInfo.token.pos).line;
if (startLine !== commaTokenLine) {
formattingScanner.advance();
tokenInfo = formattingScanner.isOnToken() ? formattingScanner.readTokenInfo(parent) : undefined;
}
}

This PR tries to fix the problem by consumeTokenAndAdvanceScanner'ing the comma instead of advanceing over it.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label May 18, 2022
@sandersn sandersn requested review from andrewbranch and gabritto May 26, 2022 21:16
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

TypeScript: Formatter deletes comment
3 participants