Skip to content
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

Add serialization error handling to settings projection layer #7576

Merged
merged 11 commits into from
Sep 11, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Sep 9, 2020

Now that CascadiaSettings is a WinRT object, we need to update the error
handling a bit. Making it a WinRT object limits our errors to be
hresults. So we moved all the error handling down a layer to when we
load the settings object.

  • Warnings encountered during validation are saved to Warnings().
  • Errors encountered during validation are saved to GetLoadingError().
  • Deserialization errors (mainly from JsonUtils) are saved to
    GetSerializationErrorMessage().

References

#7141 - CascadiaSettings is a settings object
#885 - this makes ripping out CascadiaSettings into
TerminalSettingsModel much easier

Validation Steps Performed

  • Tests passed
  • Deployment succeeded
    • tested with invalid JSON (deserialization error)
    • tested with missing DefaultProfile (validation error)

@carlos-zamora carlos-zamora requested a review from a team September 9, 2020 00:26
@carlos-zamora
Copy link
Member Author

Note to Reviewers

Feel free to review this before #7457. It's an awkward size that's...

So I'm fine either way.

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-top branch from 641e1c7 to 1885822 Compare September 9, 2020 18:04
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Rather mechanical. If it works, it works.

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/set/cascadia-settings-warnings branch from 98d08a9 to eecd112 Compare September 9, 2020 19:37
@ghost ghost closed this Sep 9, 2020
@carlos-zamora carlos-zamora reopened this Sep 9, 2020
@carlos-zamora carlos-zamora changed the base branch from dev/cazamor/set/winrt-app-obj-top to master September 9, 2020 20:51
Comment on lines 247 to 256
catch (const SettingsException& ex)
{
auto settings = winrt::get_self<implementation::CascadiaSettings>(LoadDefaults());
settings->_loadError = ex.Error();
return *settings;
}
catch (const SettingsTypedDeserializationException& e)
{
auto settings = winrt::get_self<implementation::CascadiaSettings>(LoadDefaults());
std::string_view what{ e.what() };
Copy link
Member

Choose a reason for hiding this comment

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

i'm not in love with the number of copies of this "fall back to the defaults" code there are now. this is interesting ...
it's a process that can fail, and we need a way to claw an object back out of it just to hang errors off of it.

when the consumer gets an error, what is it going to do? call LoadDefaults again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I could probably get away with just returning a blank CascadiaSettings instead? Especially since the first thing the consumer does is "if loading the settings failed --> load defaults --> display error/warning message"

This is all absolutely awful.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 11, 2020
@ghost
Copy link

ghost commented Sep 11, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 11, 2020
@DHowett DHowett merged commit 892cf05 into master Sep 11, 2020
@DHowett DHowett deleted the dev/cazamor/set/cascadia-settings-warnings branch September 11, 2020 00:57
@ghost
Copy link

ghost commented Sep 22, 2020

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants