Skip to content
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

Issue #93589: Rename 'renameShorthandProperties' setting to 'useAliasesForRenames' #94480

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

swiz115
Copy link
Contributor

@swiz115 swiz115 commented Apr 5, 2020

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.

@msftclas
Copy link

msftclas commented Apr 5, 2020

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! The name looks good but we also need to make sure migration to the new property is handled

@@ -186,7 +186,7 @@ export default class FileConfigurationManager extends Disposable {
importModuleSpecifierPreference: getImportModuleSpecifierPreference(config),
importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config),
allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file,
providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true),
providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should read both the old and new setting value for migration

Copy link
Contributor Author

@swiz115 swiz115 Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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%",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of renaming this property, create a copy of it and than mark renameShorthandProperties with a deprecated message that points to the new property

@mjbvz mjbvz added this to the April 2020 milestone Apr 6, 2020
@swiz115 swiz115 requested a review from mjbvz April 9, 2020 04:24
@@ -186,7 +186,7 @@ export default class FileConfigurationManager extends Disposable {
importModuleSpecifierPreference: getImportModuleSpecifierPreference(config),
importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config),
allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file,
providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true),
providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@swiz115 swiz115 requested a review from mjbvz April 11, 2020 03:48
@mjbvz mjbvz merged commit 3f9e684 into microsoft:master Apr 14, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented Apr 14, 2020

Thanks! Will be the next VS Code insiders build and is scheduled to go out with VS code 1.45

@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

renameShorthandProperties has confusing name
3 participants