Skip to content

Conversation

akhera99
Copy link
Member

Fixes bug where default option value was not being respected

@akhera99 akhera99 requested a review from genlu August 15, 2024 22:54
@akhera99 akhera99 requested a review from a team as a code owner August 15, 2024 22:54
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 15, 2024
// The bool setting is persisted as 0=None, 1=True, 2=False, so it needs to be retrieved as an int.
return settingManager.TryGetValue($"{CopilotOptionNamePrefix}.{optionName}", out int isEnabled) == GetValueResult.Success
&& isEnabled == 1;
&& isEnabled != 2;
Copy link
Member

@Cosifne Cosifne Aug 15, 2024

Choose a reason for hiding this comment

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

So None and True both means enabled? Does this mean it's enabled by default?

Copy link
Member

Choose a reason for hiding this comment

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

yea, this is now enabled by default (the source of truth is in copilot repo, where all those options are defined)

Copy link
Member

Choose a reason for hiding this comment

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

Copilot needs an option provider service so they can apply the defaults when you request values.

Copy link
Member

Choose a reason for hiding this comment

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

do you have an example for that?

Copy link
Member

Choose a reason for hiding this comment

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

No, it was all me wishing something existed. They could write the defaults for their settings as $"{CopilotOptionNamePrefix}.{option.Name}.Default" or something similar. Then we could defer to whatever they ship.

Copy link
Member

Choose a reason for hiding this comment

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

Why are we not using our existing options infra?

@akhera99 akhera99 enabled auto-merge (squash) August 16, 2024 05:27
@akhera99 akhera99 merged commit b0edb27 into dotnet:main Aug 16, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 16, 2024
jaredpar pushed a commit that referenced this pull request Aug 21, 2024
Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com>
@dibarbet dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants