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

Inside MainViewModel.Export(), exception handling was made using a more specific exception type #466

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

Lehonti
Copy link
Contributor

@Lehonti Lehonti commented Sep 27, 2023

This helps us get rid of the awkward variable assignment inside the try block, and we can just assign the variable directly

Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

I recommend considering updating this approach.

Before this, any exceptions other than TexParseException would be caught by our handler, but now they aren't.

I believe it's a good practice to call everything (since we know exceptions of other types may be thrown), both for the purposes of example and for practical purposes (for easier debug in example app).

Let's consider specific handling for TexParseException while still keeping general exception handling as well?

For example, we may just add a simple annotation like var message = ex is TexParseException ? $"Parse exception: {ex.Message}" : ex.Message.

What do you think?

@Lehonti
Copy link
Contributor Author

Lehonti commented Sep 27, 2023

Sure, we can do that. See if you like the updated version better

Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Thanks! Yep, I like this better.

@ForNeVeR ForNeVeR merged commit 8a93f60 into ForNeVeR:master Sep 28, 2023
4 checks passed
@Lehonti Lehonti deleted the improvement3 branch September 28, 2023 10:48
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.

2 participants