Refine extends-to-implements code fix#20991
Conversation
|
FYI @sandersn who periodically has to deal with trivia issues in code fixes and refactorings. |
There was a problem hiding this comment.
You could try something like this to avoid dealing directly with positions:
if (heritageClauses.length === 1) {
changes.replaceNode(sourceFile, extendsToken, createToken(SyntaxKind.ImplementsKeyword));
}
else {
changes.deleteNode(sourceFile, heritageClauses[0]);
// Move types from 'extends' to 'implements' clause
for (let i = heritageClauses[0].types.length - 1; i >= 0; i--) {
changes.insertNodeBefore(sourceFile, first(heritageClauses[1].types), heritageClauses[0].types[i]);
}
}(and in textChanges.ts add || isExpressionWithTypeArguments(before) to the else if in getOptionsForInsertNodeBefore)
| changes.replaceNode(sourceFile, extendsToken, createToken(SyntaxKind.ImplementsKeyword)); | ||
| changes.replaceRange(sourceFile, { pos: extendsToken.getStart(), end: extendsToken.end }, createToken(SyntaxKind.ImplementsKeyword)); | ||
| // We replace existing keywords with commas. | ||
| for (let i = 1; i < heritageClauses.length; i++) { |
There was a problem hiding this comment.
It was this way before, but shouldn't this only apply to heritageClauses[1] since the others will have commas already?
There was a problem hiding this comment.
I thought that implements I1, I2 counted as a single heritage clause (and that, in correct code, heritageClauses would have 0, 1, or 2 elements).
There was a problem hiding this comment.
OK, but then we don't need the loop, just an if.
| changes.replaceRange(sourceFile, { pos: extendsToken.getStart(), end: extendsToken.end }, createToken(SyntaxKind.ImplementsKeyword)); | ||
| // We replace existing keywords with commas. | ||
| for (let i = 1; i < heritageClauses.length; i++) { | ||
| const keywordToken = heritageClauses[i].getFirstToken()!; |
There was a problem hiding this comment.
Might want to assert that this is SyntaxKind.ImplementsKeyword.
There was a problem hiding this comment.
My thinking was that that assertion would fail in invalid code (e.g. multiple extends clauses or an out-of-order extends clause). I'm not convinced it's worthwhile to harden a best-effort code fix against that sort of bad code (the user will already be getting a diagnostic and the change is undoable), but I don't feel strongly about it. However, I'd only add the assert if we were also adding actual checks.
|
@Andy-MS I don't think I can use the |
1. Retain surrounding trivia when swapping the keyword. 2. Insert commas at the full-starts, rather than starts, of existing keywords when merging with existing implements clauses. Fixes microsoft#18794
|
Good to go? |
keywords when merging with existing implements clauses.
Fixes #18794