-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Don't use text change's createNewFile
for existing empty file
#54358
Conversation
getApplicableRefactors(fileName: string, positionOrRange: number | TextRange): ApplicableRefactorInfo[] { | ||
const args = this.createFileLocationOrRangeRequestArgs(positionOrRange, fileName); | ||
|
||
getApplicableRefactors( |
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.
Fourslash server tests previously didn't work with move to file
@@ -652,7 +652,7 @@ export interface LanguageService { | |||
* arguments for any interactive action before offering it. | |||
*/ | |||
getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined, triggerReason?: RefactorTriggerReason, kind?: string, includeInteractiveActions?: boolean): ApplicableRefactorInfo[]; | |||
getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string, preferences: UserPreferences | undefined, includeInteractiveActions?: InteractiveRefactorArguments): RefactorEditInfo | undefined; |
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.
Drive-by fix, I thought this parameter had a misleading name, since it's not a boolean.
@@ -218,6 +226,9 @@ function getNewStatementsAndRemoveFromOldFile( | |||
if (targetFile.statements.length > 0) { | |||
changes.insertNodesAfter(targetFile, targetFile.statements[targetFile.statements.length - 1], body); | |||
} | |||
else { | |||
changes.insertNodesAtEndOfFile(targetFile, body, /*blankLineBetween*/ false); | |||
} | |||
if (imports.length > 0) { | |||
insertImports(changes, targetFile, imports, /*blankLineBetween*/ true, preferences); |
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.
Regarding the extra new lines, we could also change this code to pass blankLineBetween: false
, and the consequence would be that we'd sometimes not have an empty line between the import blocks and the moved statements in the target file, e.g.:
import { a } from "./a";
const x = 2;
instead of
import { a } from "./a";
const x = 2;
src/harness/client.ts
Outdated
@@ -1,3 +1,4 @@ | |||
import { GetApplicableRefactorsRequestArgs, GetEditsForRefactorRequestArgs } from "../server/protocol.js"; |
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.
Nit: protocol
is already imported from a namespace file, and I think that may still matter for initialization order in some cases.
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.
Why did a .js extension show up here?
(I would use the existing import if possible)
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.
I assume this was auto-imports with non-default VS Code settings (importModuleSpecifierEnding=js
)
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.
Yep, I had importModuleSpecifierEnding
set to js
on my user settings (I think I accidentally changed the user one while trying to repro a bug in the past). Thanks for pointing that out
@typescript-bot cherry-pick this to release-5.1 |
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
Hey @DanielRosenwasser, I've opened #54444 for you. |
…e-5.1 (#54444) Co-authored-by: Gabriela Araujo Britto <gabrielaa@microsoft.com>
Fixes #54285.
This fix updates our handling of move to file when the target file already exists, but doesn't yet have any statements (i.e. is "empty"). Previously, we'd treat this empty file as a new file for the purposes of writing text changes to it, but this caused the reported crash, and it also had the consequence of us overwriting possibly existing comments in this empty target file.
This fix, however, uncovered a problem with how new lines are handled when we insert imports into a target file in move to file. The issue was already present, but now it also occurs when the target is an empty file. The issue is that we end up with extra new lines between the import statements the refactoring inserts, as witnessed by the test baselines that changed in this PR. There is also an inconsistency between choice of quotes that shows up in the tests. Both problems seem to be caused by us using
ImportAdder
to add some of the imports in the target file, but also adding imports not using theImportAdder
, and it seems like ideally we'd unify that so that we're consistent with new lines and quotes.