`error-chain` -> `thiserror` port

aerospike / aerospike-client-rust

Rust client for the Aerospike database

https://www.aerospike.com/

Other

82 stars 26 forks source link

`error-chain` -> `thiserror` port #131

Open databasedav opened 1 year ago

databasedav commented 1 year ago

@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 commented 1 year ago

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

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.
We have to implement it ourselves. This example could be helpful: https://github.com/jsvana/async-minecraft-ping/pull/8/files
No need for bail!. The idea with moving to thiserror was to have idiomatic error handling.

databasedav commented 1 year ago

@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 commented 1 year ago

@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 commented 5 months ago

@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.