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

change to camelCase for Default, Enabled, Disabled #9862

Merged
merged 15 commits into from
Sep 20, 2022

Conversation

browntarik
Copy link
Contributor

@browntarik browntarik commented Sep 8, 2022

change casing to reflect changes made in adding the case sensitive file support setting

Copy link
Member

@bobbrow bobbrow left a comment

Choose a reason for hiding this comment

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

A couple of 'default' values in the package.json need to be updated.

When doing string comparisons, you will need to lowercase the value before checking against the new lowercase value because some people might still have a PascalCase value for the setting. I didn't mark all the places that need to be updated.

Extension/package.json Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
@bobbrow bobbrow requested a review from a team September 8, 2022 17:35
Extension/package.json Outdated Show resolved Hide resolved
Extension/src/Debugger/configurationProvider.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
Extension/src/main.ts Show resolved Hide resolved
Extension/src/main.ts Show resolved Hide resolved
Extension/src/main.ts Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/package.nls.json Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
@@ -158,7 +158,7 @@ export class CustomConfigurationProviderCollection {
}

public add(provider: CustomConfigurationProvider, version: Version): boolean {
if (new CppSettings(ext.getActiveClient().RootUri).intelliSenseEngine === "Disabled") {
if (new CppSettings(ext.getActiveClient().RootUri).intelliSenseEngine === "disabled") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, you could lower case these in the getters in settings.ts so they always return lower case strings. Then all comparisons between settings originating from these getters would not need any conversions. That would seem simpler, as we'd need to deal with potentially different casing in much fewer places (no other places?).

Also, for setters that are simply enabled/disabled, it would be most efficient if they were converted into bools by the getters in settings.ts, so could thereafter be evaluated as bools instead of needing to do any more expensive string conversions/comparisons.

@sean-mcmanus
Copy link
Collaborator

@bobbrow Can you re-review/approve this? It says you're requesting changes still.

Extension/package.json Outdated Show resolved Hide resolved
Extension/package.json Outdated Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
@bobbrow bobbrow dismissed their stale review September 19, 2022 19:47

Not blocking for the internal rename

@bobbrow
Copy link
Member

bobbrow commented Sep 19, 2022

I removed my block on this PR, but the 2 package.json when clauses need to be updated before this PR goes in.

Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
Extension/src/Debugger/configurationProvider.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/customProviders.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
Extension/src/LanguageServer/settings.ts Outdated Show resolved Hide resolved
Extension/src/main.ts Outdated Show resolved Hide resolved
Extension/src/main.ts Outdated Show resolved Hide resolved
Extension/src/main.ts Outdated Show resolved Hide resolved
Extension/src/main.ts Outdated Show resolved Hide resolved
@@ -2158,11 +2158,11 @@
"C_Cpp.intelliSenseEngine": {
"type": "string",
"enum": [
"Default",
"default",
"Tag Parser",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the "Tag Parser" enum? That seems inconsistent now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should it be "tagParser"? The C++ side might need updating.

"Default",
"Disabled"
"default",
"disabled"
],
"markdownEnumDescriptions": [
"%c_cpp.configuration.formatting.clangFormat.markdownDescription%",
"%c_cpp.configuration.formatting.vcFormat.markdownDescription%",
"%c_cpp.configuration.formatting.Default.markdownDescription%",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe these string for the enum descriptions should be updated too?

Copy link
Collaborator

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I made some comments

@browntarik browntarik merged commit 230bce9 into main Sep 20, 2022
@browntarik browntarik deleted the browntarik/changePascalCase branch September 20, 2022 21:13
@sean-mcmanus sean-mcmanus mentioned this pull request Sep 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2022
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.

6 participants