SerializableOptionSet and IOptionService cleanup#61839
Conversation
8e0b751 to
9b54569
Compare
| /// character. The position provided is the position of the caret in the document after | ||
| /// the character been inserted into the document. | ||
| /// </summary> | ||
| [Obsolete("Use the other overload")] |
| ValidateOrganizeImportsOptions(await Formatter.GetOrganizeImportsOptionsAsync(csDocumentWithUpdatedOptions, CancellationToken.None)); | ||
| ValidateOrganizeImportsOptions(await Formatter.GetOrganizeImportsOptionsAsync(vbDocumentWithUpdatedOptions, CancellationToken.None)); | ||
|
|
||
| static OptionSet GetOptionSetWithChangedPublicOptions(OptionSet options) |
| namespace Microsoft.CodeAnalysis.UnitTests | ||
| { | ||
| internal static class TestOptionService | ||
| { |
There was a problem hiding this comment.
Methods moved to src/Workspaces/CoreTest/WorkspaceServiceTests/GlobalOptionServiceTests.cs
| _changedOptionKeysNonSerializable.Add(optionKey)); | ||
| _globalOptions, | ||
| _values.SetItem(optionKey, value), | ||
| _changedOptionKeys.Add(optionKey)); |
There was a problem hiding this comment.
The uses of _values and _changedOptionKeys was a little confusing to me at first. Am wondering if it would make sense to store a single map of changedKey:changedValue (assuming of course that the _globalOptions.GetOption doesn't have perf concerns).
There was a problem hiding this comment.
Yeah, so I was being careful here. _globalOptions.GetOption is locking and reading/writing option value to persisters. I think the reason why we have separate _changedOptionKeys is that the _values serve as a cache for options that are read from the global options but not changed.
There was a problem hiding this comment.
also a bit unclear to me. consider doc'ing more.
| Debug.Fail($"Failed to deserialize: {name}-{feature}-{isPerLanguage}-{language}"); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
this makes me so happy
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
remove :) (i can't suggest an edit for some reason).
| /// Gets force computed serializable options snapshot with prefetched values for the registered options applicable to the given <paramref name="languages"/> by quering the option persisters. | ||
| /// </summary> | ||
| SerializableOptionSet GetSerializableOptionsSnapshot(ImmutableHashSet<string> languages, IOptionService optionService); | ||
| SerializableOptionSet GetOptions(IOptionService optionService); |
There was a problem hiding this comment.
might be good to have a doc comment on this.
| WorkspaceOptionSet workspaceOptionSet, | ||
| ImmutableDictionary<OptionKey, object?> values, | ||
| ImmutableHashSet<OptionKey> changedOptionKeysSerializable, | ||
| ImmutableHashSet<OptionKey> changedOptionKeysNonSerializable) |
There was a problem hiding this comment.
what's the current use case for changedOptionKeysNonSerializable? how does that work? (feel free to not have an answer... i'm just still uncertain about those guys).
|
|
||
| return Comparer.Default.Compare(x.GetHashCode(), y.GetHashCode()); | ||
| } | ||
| } |
There was a problem hiding this comment.
this hte best PR ever.
| /// It contains prepopulated fetched option values for all serializable options and values, and delegates to <see cref="WorkspaceOptionSet"/> for non-serializable values. | ||
| /// It ensures a contract that values are immutable from this instance once observed. | ||
| /// </summary> | ||
| internal sealed partial class SerializableOptionSet : OptionSet |
There was a problem hiding this comment.
consider adding doc comment back that explains what the purpose o fthis type actualy is.
| private readonly ImmutableHashSet<OptionKey> _changedOptionKeysNonSerializable; | ||
| private ImmutableDictionary<OptionKey, object?> _values; | ||
|
|
||
| private readonly ImmutableHashSet<OptionKey> _changedOptionKeys; |
src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs
Show resolved
Hide resolved
|
Overall fantastic. Def have some calrifying questions, and requests for docs. Anything we expect to be a long lived type in our repo, i'd like some docs on. If there are other types we expect are going to go away shortly, then we don't need to doc, but i'd like clarity on which fall into that bucket and if we're going to tackle them soon. Thanks! |
78e13b9 to
7421dd7
Compare
7421dd7 to
ca61166
Compare
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
This reverts commit 2f9faa0.
Remove code that's no longer needed.
Recommended to review commit by commit.