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

Reduce error boilerplate using thiserror #83

Open
jcgruenhage opened this issue Apr 8, 2023 · 2 comments
Open

Reduce error boilerplate using thiserror #83

jcgruenhage opened this issue Apr 8, 2023 · 2 comments

Comments

@jcgruenhage
Copy link
Contributor

During the port from attohttpc to reqwest, I've noticed that there's a bunch of boilerplate code around error handling. A lot of the formatting/From impls etc can be reduced a lot by using stuff like https://lib.rs/crates/thiserror.

@jcgruenhage
Copy link
Contributor Author

I've started taking a jab at this, I've identified a few more problems that I didn't see before:

  • There's a lot of type information lost in the current error handling. When handling. Each time an error type is converted using the ? operator, it is converted into a string. In my local draft, I've fixed this by having a big enum with the types to convert from kept in variants. All the From impls are generated by thiserror
  • Context for errors is kept in the same String as the error itself, by using a prefix method, which prepends a string to the error message itself. This means that even with the changes I've made before, when using the prefix method, I have to convert the error to a generic string error type, loosing the type information again. There's crates specialized on adding context to errors, like anyhow.
  • There's a gigantic amount of error sources that all get dumped into a flat error type. I'd think it would make sure to restructure this a bit and split them into a few smaller error types, similar to what we already have for AcmeError and HttpError, but to extend that to also add a few errors for stuff like config loading etc.

So, now with a bit of my findings documented, there's just one question I have: Do we want to keep type information of the errors we're wrapping? If so, the approach with thiserror I've started with sounds like a sensible option to me, but it's a lot more work to structure it properly. If not, we can get rid of nearly all of the error handling code if we just switch to anyhow.

@jcgruenhage
Copy link
Contributor Author

Regarding the commit that just landed on main last night (e9522df), I think it'd make sense to push changes to a branch and open a PR, even when working on your own projects. This gives people the opportunity to provide feedback before things land on main, and it allows the CI to run, catching things one might have missed locally, such as the MSRV increase in df40cde.

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

No branches or pull requests

1 participant