Skip to content

Simplify code for reading/writing settings#81515

Merged
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:unifiedSettingsSimplification2
Dec 2, 2025
Merged

Simplify code for reading/writing settings#81515
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:unifiedSettingsSimplification2

Conversation

@CyrusNajmabadi
Copy link
Contributor

Realized in the shower that i made things too complicated. Simplifying greatly.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner December 2, 2025 13:19

internal Optional<object?> TryReadOptionValue(string storageKey, Type storageType, object? defaultValue)
{
if (storageType == typeof(bool))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this reading is specific to the legacy settings codepath. It's how roslyn encoded specific values into the legacy settings store. it doesn't apply to the new unified settings system, and we don't want to actually share this code.

Note: youc an tell this was worthless becausein the PR i merged in, the unified settings path always converts the value to write into a string. So we would first do that, and then call into this helper to just call into Read<string>(); and do nothing else.

private Task SetValueAsync(string storageKey, object? value)
=> this.SettingsManager.SetValueAsync(storageKey, value, isMachineLocal: false);

internal override Optional<object?> TryReadOptionValue(OptionKey2 optionKey, string storageKey, Type storageType, object? defaultValue)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is all just a move.

Debug.Assert(typeof(T) == typeof(string));
var retrieval = this.SettingsManager.GetReader().GetValue<T>(
storageKey, SettingReadOptions.NoRequirements);
if (storageType == typeof(int))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the unified-settings codepath now gets simpler as well. it just handles ints (for tab-size/indent-size), and enums (for use-spaces, and indent-style).

If we move more things here, we can expand this out as appropriate.

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Looks like the unified settings persister will need to support booleans for indent_style and the tests will need to validate against the correct types instead of strings.

@CyrusNajmabadi
Copy link
Contributor Author

@JoeRobich right yuo are. i had tests for this, but they were in the wrong namespace, so i wasn't seeing they were failing locally :D

@JoeRobich
Copy link
Member

/azp run roslyn-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@CyrusNajmabadi CyrusNajmabadi merged commit 57e55e9 into dotnet:main Dec 2, 2025
25 of 26 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the unifiedSettingsSimplification2 branch December 2, 2025 21:45
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 2, 2025
@davidwengier davidwengier modified the milestones: Next, 18.3 Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants