Repository navigation
Fix trailing formatting edit when range ends mid-token - #50082
Merged
Andrew Branch (andrewbranch) merged 1 commit intoJul 28, 2022
Merged
Conversation
Andrew Branch (andrewbranch)
marked this pull request as ready for review
July 28, 2022 18:16
TypeScript Bot (typescript-bot)
added
Author: Team
For Milestone Bug
Andrew Branch (andrewbranch)
requested review from
Gabriela Araujo Britto (gabritto) and
Jake Bailey (jakebailey)
July 28, 2022 18:16
| // with a selection from the beginning of the line to the space inside the curly braces, | ||
| // inclusive. We would expect a format-selection would delete the space (if rules apply), | ||
| // but in order to do that, we need to process the pair ["{", "}"], but we stopped processing | ||
| // just before getting there. This block handles this trailing edit. |
Member
Author
There was a problem hiding this comment.
This comment should have been included with #46875.
| // produce bogus edits if we try to `processPair`. Recall that the point of this logic is | ||
| // to perform a trailing edit at the end of the selection range: but there can be no valid | ||
| // edit in the middle of a token where the range ended, so if we have a non-contiguous | ||
| // pair here, we're already done and we can ignore it. |
Member
Author
There was a problem hiding this comment.
This comment describes the fix for this bug.
Jake Bailey (jakebailey)
approved these changes
Jul 28, 2022
Jake Bailey (jakebailey)
left a comment
Jake Bailey (jakebailey)
left a comment
Member
There was a problem hiding this comment.
LGTM, I'm always suspicious of the ! but they're at least consistent with the other side-effecty previous variables, so...
Member
Author
|
Yeah, it’s correlated with the existence of |
Comment on lines
+451
to
+452
| // but in order to do that, we need to process the pair ["{", "}"], but we stopped processing | ||
| // just before getting there. This block handles this trailing edit. |
There was a problem hiding this comment.
Suggested change
| // but in order to do that, we need to process the pair ["{", "}"], but we stopped processing | |
| // just before getting there. This block handles this trailing edit. | |
| // but in order to do that, we need to process the pair ["{", "}"]. | |
| // This block handles this trailing edit by ensuring we continue processing. | |
| // the pair of tokens that the range intersects. |
|
|
||
| if (tokenInfo) { | ||
| if (tokenInfo && tokenInfo.pos === previousRangeTriviaEnd!) { | ||
| // We need to check that tokenInfo and previousRange are contiguous: the `originalRange` |
There was a problem hiding this comment.
Suggested change
| // We need to check that tokenInfo and previousRange are contiguous: the `originalRange` | |
| // We need to check that previousRange and tokenInfo are contiguous since the `originalRange` |
Daniel Rosenwasser (DanielRosenwasser)
approved these changes
Jul 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #50076