replaceNode: Always use non-adjusted end #22519
Conversation
|
I believe the adjusted end position includes trailing comments, not just the newline. Also, why would the start and the end not be symmetric? |
|
Yeah, looks like we should never be adjusting the start position either. |
| * If pos\end should be interpreted literally 'useNonAdjustedStartPosition' or 'useNonAdjustedEndPosition' should be set to true | ||
| */ | ||
| export type ConfigurableStartEnd = ConfigurableStart & ConfigurableEnd; | ||
| export interface ConfigurableStartEnd extends ConfigurableStart, ConfigurableEnd {} |
There was a problem hiding this comment.
Converting to an interface because of #19296
|
@amcasey Could you re-review? |
|
This change only partially resolves #21246, correct? |
src/services/textChanges.ts
Outdated
| const pos = getAdjustedStartPosition(sourceFile, oldNode, options, Position.Start); | ||
| const end = getAdjustedEndPosition(sourceFile, oldNode, options); | ||
| return this.replaceRange(sourceFile, { pos, end }, newNode, options); | ||
| public replaceNode(sourceFile: SourceFile, oldNode: Node, newNode: Node, options: InsertNodeOptions = {}) { |
There was a problem hiding this comment.
I was only expecting the default to change but this looks like it makes the alternative (i.e. adjusting start and end positions) impossible. Was that intentional?
There was a problem hiding this comment.
Also it seems strange to update this signature without making corresponding changes to (e.g.) replaceNodeRange.
There was a problem hiding this comment.
I don't think we're likely to have any reason to replace the a node's comments while replacing it -- if we do end up needing that, we can add it back.
There was a problem hiding this comment.
I guess I think of replace(Node | Range | NodeRange)(WithNodes)? as a family of related functions that should have parallel signatures. Notice that the WithNodes variants already default to using non-adjusted positions, which should make things easier.
There was a problem hiding this comment.
Added the option back where it makes sense, but not for replaceRange which should only accept InsertNodeOptions -- it doesn't adjust pos and end since it accepts a range.
There was a problem hiding this comment.
Created #22813 which will be easier to review independently, then can merge into this.
| this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " }); | ||
| } | ||
|
|
||
| public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string): void { |
There was a problem hiding this comment.
I'm surprised that we need to compute line numbers for this operation (I would have guessed, perhaps incorrectly, that that was expensive), but I see that this is just a refactoring.
There was a problem hiding this comment.
It could probably be done without the line numbers by counting back to the beginning of the line. Though, getting the line number is just a binary search given that sourceFile.lineMap is cached.
ghost
force-pushed
the
replaceNode_end
branch
2 times, most recently
from
65184f1 to
a6be0a6
Compare
ghost
mentioned this pull request
Alternative to #22517 -- there are no remaining uses where we need to adjust an end position with
replaceNode, so make that the hardcoded behavior. In some cases we were adjusting the end, then adding{ suffix: newLineCharacter }, which means we remove and then replace the newline.