Allow per-file setting for rename default behavior preferences #29593
Conversation
|
FYI @mjbvz |
|
Thanks for the heads up @uniqueiniquity. Are you planning to take this in for 3.3.1? |
src/server/session.ts
Outdated
| @@ -1178,7 +1178,7 @@ namespace ts.server { | |||
| private getRenameInfo(args: protocol.FileLocationRequestArgs): RenameInfo { | |||
| const { file, project } = this.getFileAndProject(args); | |||
| const position = this.getPositionInFile(args, file); | |||
| const preferences = this.getHostPreferences(); | |||
| const preferences = { ...this.getHostPreferences(), ...this.getPreferences(file) }; | |||
There was a problem hiding this comment.
Is there a reason we shouldn't just change
getPreferences(file: NormalizedPath): protocol.UserPreferences {
const info = this.getScriptInfoForNormalizedPath(file);
return info && info.getPreferences() || this.hostConfiguration.preferences;
}in editorServices.ts into
getPreferences(file: NormalizedPath): protocol.UserPreferences {
const info = this.getScriptInfoForNormalizedPath(file);
return { ...this.hostConfiguration.preferences, ...info && info.getPreferences()};
}? (And just use this.getPreferences(file) here)
There was a problem hiding this comment.
Good idea. Changed to that.
src/server/session.ts
Outdated
| @@ -1178,7 +1178,7 @@ namespace ts.server { | |||
| private getRenameInfo(args: protocol.FileLocationRequestArgs): RenameInfo { | |||
| const { file, project } = this.getFileAndProject(args); | |||
| const position = this.getPositionInFile(args, file); | |||
| const preferences = this.getHostPreferences(); | |||
| const preferences = { ...this.getHostPreferences(), ...this.getPreferences(file) }; | |||
There was a problem hiding this comment.
I am not sure whats the intent here with preference per file? Rename is an operation that affects multiple files. So even though rename originated in say file A (whose preferences we are checking as per this PR), what happens to rename locations in B.. Shouldn't we prefer rename preferences of B for the locations in that file instead ?
There was a problem hiding this comment.
@mjbvz, what do you think? I agree that this setup is a bit odd for this particular operation, but it would definitely be more complicated to give each file the agency to decide for itself which rename location strategy to use.
There was a problem hiding this comment.
Discussed with @mjbvz offline. He was in favor of the originating file determining the behavior of the whole operation. I've added a test and comment to make this clear.
|
Can we add a couple fourslash tests? |
|
@RyanCavanaugh relevant fourslash test are in the PRs mentioned above (particularly #29314). |
|
@sheetalkamat @weswigham I've addressed your comments; let me know if there's anything else. |
Fixes #29585.
#29314 and #29385 made it so their respective settings are only recognized when provided to the host as a whole.
This PR makes it so that the relevant settings for the preferences on the file override those of the preferences on the host.