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

error-chain -> thiserror port #131

Draft
wants to merge 3 commits into
base: async
Choose a base branch
from

Conversation

databasedav
Copy link

No description provided.

@databasedav
Copy link
Author

@khaf i believe i'm almost done here but need some guidance on a few things

  • what to do about log_error_chain? the thiserror Error type doesn't expose a .backtrace method like error-chain does, although i believe we can get a similar effect with Backtrace::force_capture
  • how to handle error contexts? this was builtin to error-chain with the .chain_err method, which thiserror doesn't have an analog for, and while anyhow does have "context" methods which serve the same purpose, they return anyhow::Error wrappers as opposed to raw the concrete error from thiserror, which relates to the last concern ...
  • i went ahead and replaced the bail! calls with return Err to avoid either 1. creating a custom bail! just for this library, or 2. having to introduce anyhow which has a general bail!, which would similarly have the negative of introducing another crate that add another layer to the thiserror error enum

@khaf
Copy link
Collaborator

khaf commented May 16, 2023

@databasedav Thanks for the quick turn around! I looked around for a few hours and from what I understand thus far:

  1. There is no easy workaround. thiserror uses an unstable feature for backtraces that until final release can only be used via nightly. I have not been able to come up with a solution myself. Maybe @jonas32 can chime in.
  2. We have to implement it ourselves. This example could be helpful: https://github.com/jsvana/async-minecraft-ping/pull/8/files
  3. No need for bail!. The idea with moving to thiserror was to have idiomatic error handling.

@databasedav
Copy link
Author

databasedav commented May 16, 2023

@khaf thoughts on using error-stack? i'm not a big fan of how the "context nesting" strategy pollutes the library level with having to deal with the Generic/WithContext error wrappers, embedding the context api into the error struct with a trait is much cleaner/more idiomatic imo

@khaf
Copy link
Collaborator

khaf commented May 31, 2023

@databasedav I'm pretty sure I had answered this right away, but seeing that my response is missing, I assume I left to research and reflect and forgot to come back to it. Sorry about that.

I have nothing against the error-stack. As long as the library does not impose runtime cost, leaking abstractions and limitation upon us, I'm fine with it.

I'm open to the idea, let me know how it goes.

@khaf
Copy link
Collaborator

khaf commented Apr 8, 2024

@databasedav I have updated this PR to finish the work you started. The code compiles now.
Having looked at error-stack, it shares an issue with anyhow in that it shows up in the public API. It is also too complicated for our needs, and has a much smaller user base in comparison to thiserror. The current implementation is not complete, since I'd like to add backtrace and source to all errors to easy debugging, but I think we are on the right track.
To chain errors, I have introduced a Wrapped option, which boxes errors inside in a chain. I will have to rename it to something better (Chain maybe? Open to suggestions) and implement some public API to allow easy introspection.
Let me know what you think.

@khaf khaf mentioned this pull request Apr 8, 2024
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