Issue #93589: Rename 'renameShorthandProperties' setting to 'useAliasesForRenames'#94480
Issue #93589: Rename 'renameShorthandProperties' setting to 'useAliasesForRenames'#94480mjbvz merged 3 commits intomicrosoft:masterfrom
Conversation
…'useAliasesForRenames'
|
|
mjbvz
left a comment
mjbvz
left a comment
There was a problem hiding this comment.
Thanks for taking a look! The name looks good but we also need to make sure migration to the new property is handled
| importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config), | ||
| allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file, | ||
| providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true), | ||
| providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true), |
There was a problem hiding this comment.
This should read both the old and new setting value for migration
There was a problem hiding this comment.
Thanks you guidance! This was an attempt at my first PR. I'll update this soon and keep this in mind with my future contributions.
There was a problem hiding this comment.
Until we remove the deprecated setting, we should make sure we respect previous setting value. Something like:
config.get<boolean>('renameShorthandProperties', true) === false ? false : config.get<boolean>('useAliasesForRenames', true)There was a problem hiding this comment.
I apologize. Not sure how I missed this in the previous attempt at fixing my first commit.
| "type": "boolean", | ||
| "default": true, | ||
| "description": "%typescript.preferences.renameShorthandProperties%", | ||
| "description": "%typescript.preferences.useAliasesForRenames%", |
There was a problem hiding this comment.
Instead of renaming this property, create a copy of it and than mark renameShorthandProperties with a deprecated message that points to the new property
| importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config), | ||
| allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file, | ||
| providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true), | ||
| providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true), |
There was a problem hiding this comment.
Until we remove the deprecated setting, we should make sure we respect previous setting value. Something like:
config.get<boolean>('renameShorthandProperties', true) === false ? false : config.get<boolean>('useAliasesForRenames', true)
|
Thanks! Will be the next VS Code insiders build and is scheduled to go out with VS code 1.45 |
3 participants
This PR fixes #93589
The original setting name 'renameShorthandProperties' represents what the setting will do when disabled—not what it will do when it's enabled (the default). Changing the setting name to 'useAliasesForRenames' is better suited.