-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Fixing range for primary edit #59369
Fixing range for primary edit #59369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another call to getText()
below that is incorrectly used. It's arguable whether or not that should just reuse originalText
, but I honestly am not familiar enough with the code.
But something must be wrong when copying/pasting trivia over, and the new test must not cover it.
I haven't found a case where the next |
|
||
this.getScriptInfo(rootFile)?.editContent(0, originalText.length, updatedText); | ||
this.updateGraph(); | ||
try { | ||
cb(this.program!, originalProgram, (this.program?.getSourceFile(rootFile))!); | ||
} | ||
finally { | ||
this.getScriptInfo(rootFile)?.editContent(0, this.program!.getSourceFile(rootFile)!.getText().length, originalText); | ||
this.getScriptInfo(rootFile)?.editContent(0, updatedText.length, originalText); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... looking at it again, is it really safe to assume that cb
doesn't itself change the contents of the file? I'm sure the way we use this function is used in our codebase, it's fine, but I just want to check. What do you think @sheetalkamat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should assume cb doesnt change the contents of file since file is never "muted" without change in script info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(assuming you mean mutated)
What I meant was that cb
could itself run editContent
somehow. If that happens, the ScriptInfo
object may not actually contain the same text anymore.
I'm okay with assuming that will never happen for now, I just wanted to run it by you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is just to replace the text contents with the script info's own text contents. It should all be available anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did mean mutated
This reverts commit b04c8a0.
This reverts commit 0d5380b.
Fixes #58936
Due to
getText()
and notgetFullText()
, new line characters were getting ignored when trying to perform a temporary file update which later made the paste at the wrong positions.