Issue #93589: Rename 'renameShorthandProperties' setting to 'useAliasesForRenames' by swiz115 · Pull Request #94480 · microsoft/vscode

Issue #93589: Rename 'renameShorthandProperties' setting to 'useAliasesForRenames' by swiz115 · Pull Request #94480 · microsoft/vscode

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.

All CLA requirements met.

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

		importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config),
		allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file,
		providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true),
		providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true),

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

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.

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)

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%",

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

		importModuleSpecifierEnding: getImportModuleSpecifierEndingPreference(config),
		allowTextChangesInNewFiles: document.uri.scheme === fileSchemes.file,
		providePrefixAndSuffixTextForRename: config.get<boolean>('renameShorthandProperties', true),
		providePrefixAndSuffixTextForRename: config.get<boolean>('useAliasesForRenames', true),

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)

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