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

errors instead of panics #275

Merged
merged 8 commits into from
Aug 31, 2020
Merged

errors instead of panics #275

merged 8 commits into from
Aug 31, 2020

Conversation

Johann150
Copy link
Contributor

resolves #189

This is just a first idea of how it could look like. One point right away to consider is when some of these unwraps might fail and if the errors should be more specific then.

Something else is also that std::io::Error has to be wrapped. Maybe there is a nicer solution for that.

@Johann150 Johann150 marked this pull request as ready for review August 21, 2020 20:44
@brendanzab
Copy link
Owner

Nice! Thanks for looking into this. Does this remove all our unwraps?

@Johann150
Copy link
Contributor Author

There are still unwraps in

  • some tests
  • the examples
  • codespan-lsp, although that crate uses its own error enum

found this while looking for any left over unwraps
@brendanzab
Copy link
Owner

Yeah I think it's fine to ignore those files for now - the main point of the issue is to tackle the stuff in codespan-reporting.

@brendanzab
Copy link
Owner

One thing that gave me pause when attempting to do this in the past was wondering if I needed to let users define their own error types on the Files trait. I'm not sure how much of an issue that is in practice though. And we could always add that later I guess, if the need was there?

Co-authored-by: Brendan Zabarauskas <[email protected]>
@Johann150
Copy link
Contributor Author

We could implement something similar to Io with a generic type or a trait.

Co-authored-by: Markus Westerlind <[email protected]>
@brendanzab brendanzab merged commit 593ed53 into brendanzab:master Aug 31, 2020
@brendanzab
Copy link
Owner

Ok, merged this! We might want to add some more descriptive information later, but I think this is good.

@Johann150 Johann150 deleted the patch-4 branch August 31, 2020 09:38
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.

Return error values rather than panicking when rendering diagnostics.
3 participants