-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Rework JsonUtils' optional handling to let Converters see null #8175
Conversation
struct hstring_like | ||
{ | ||
std::string value; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, was there something in the test that made this unclear?
These test failures are not caused by this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this so much. Double approve haha
{ \ | ||
return *val.setting; \ | ||
return **val; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe
Since this changed the behavior of some more settings, it'll require @zadjii-msft to sign off. 😄 |
@@ -201,12 +197,12 @@ private: \ | |||
/*iterate through parents to find a value*/ \ | |||
for (auto parent : _parents) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (auto parent : _parents) \ | |
for (const auto& parent : _parents) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk how we missed this in code review
shouldn't we have had a helper for "walk parents and do a thing"? I thought that was part of the brief.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I ran into by taking this out into its own helper method was that we're returning a value and calling a specific _get##name##Impl()
. Figured it'd be less of a headache to throw this in a macro.
The const &
thing is my bad tho. Sorry! I think I went through all of the other ones in the PR, but I just forgot to check the macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I feel good about this because there are good tests for this, not because I understand templates
🎉 Handy links: |
The JsonUtils changes in #8018 revealed that we need more robust,
configurable optional handling. We learned that there's a class of
values that was previously underrepresented in our API: strings that
have an explicit empty value.
The Settings model supports starting directory, icon, background image
et al values that are empty. That emptiness overrides a value set in a
lower layer, so it is not sufficient to represent the empty value for
any one of those fields as an unset optional.
There are a couple other settings for which we've implemented a
hand-rolled option type (for roughly the same reason): foreground,
background, any color fields that override values from the color scheme
or the lower layer profile.
These requirements are best fulfilled by better optional support in
JsonUtils. Where the library would originally detect known types of
optional and pre-filter them out during
GetValue
andSetValue
, itwill now defer to another conversion trait.
This commit introduces a helper conversion trait and an "option oracle".
The conversion trait will use the option oracle to detect emptiness,
generate empty option values, and read values out of option types. In so
doing, the trait is insulated from the implementation details of any
specific option type.
Any special logic for handling JSON null and option types has been
stripped from GetValue. Due to this, there is an express change in
behavior for some converters:
GetValue<T>(jsonNull)
whereT
is not an option type[1] hasbeen upgraded from a silent no-op to an exception.
Further, I took the opportunity to replace NullableSetting with
std::optional<std::optional>, which accurately represents "setting
that the user might explicitly clear". I've added a test to
JsonUtilsTests to make sure it can serialize/deserialize double
optionals the way we expect it to.
Tests (Local, Unit for TerminalApp/SettingsModel):
Summary: Total=140, Passed=140, Failed=0, Blocked=0, Not Run=0, Skipped=0
[1]: Explicitly, if
T
is not an option type and the converter doesnot support null.