Simplify code for reading/writing settings#81516
Conversation
|
|
||
| protected bool TryFetchWorker(OptionKey2 optionKey, string storageKey, Type optionType, out object? value) | ||
| { | ||
| var result = TryReadAndMonitorOptionValue(optionKey, storageKey, storageKey, optionType, optionKey.Option.DefaultValue); |
There was a problem hiding this comment.
we passed this value along just to do a type check and call into a helper method. but we can determine that from the optionType passed in.
| return default; | ||
| } | ||
|
|
||
| if (defaultValue is ICodeStyleOption2 codeStyle) |
There was a problem hiding this comment.
this was the only actual use of defaultValue. Note: i very VERY much want this value gone because it's very tempting to return that value in the case of failure. But that's not the behavior we want. we want to actually return nothing as the final high level caller needs to know if needs to go to the next persister.
so this makes sure that mistake can't happen (which i s something i did, but fortunately had a test catch).
|
@JoeRobich this is ready for review. tnx. |
| try | ||
| { | ||
| return new Optional<object?>(codeStyle.FromXElement(XElement.Parse(value))); | ||
| var fromXElementMember = storageType.GetMethod(nameof(CodeStyleOption2<>.FromXElement), BindingFlags.Public | BindingFlags.Static); |
There was a problem hiding this comment.
I've been taught to be wary of reflection. Is this something that runs frequently or only on start up and when an option's value changes?
There was a problem hiding this comment.
yes. these values, once read, are cached in the higher level construct. then we only clear that cached value when we get a notification that the value changed, and we only refetch it at that point.
Followup to #81515