Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Handle errors with thiserror #2443

Draft
wants to merge 2 commits into
base: 1-3
Choose a base branch
from

Conversation

suchapalaver
Copy link
Contributor

Signed-off-by: Joseph Livesey joseph.livesey@btp.works SAW-13

Signed-off-by: Joseph Livesey <joseph.livesey@btp.works>
Signed-off-by: Joseph Livesey <joseph.livesey@btp.works>
@vaporos
Copy link
Contributor

vaporos commented Mar 21, 2023

I'm not a fan of pulling in thiserror - errors are not difficult enough to warrant a dependency (despite us trying to make them more difficult by creating too many of them). That said, most the changes here have nothing to do with thiserror per-se, and I agree with the overall cleanups.

We should use the errors in libsawtooth (which were taken from splinter) - https://github.com/hyperledger/sawtooth-lib/tree/main/libsawtooth/src/error . Basically, InternalError, InvalidArgumentError, and InvalidStateError handle ~98% of all errors we ever need, with the 2% generally being compound errors containing 1 or more of those in an enum (and the occasional custom error when we actually need to return additional information inside the error). Probably 90%+ of everything in sawtooth would be InternalError - i.e. an error where the caller only knows something went wrong, but can't handle it in any specific way. This has worked extremely well in projects were we've taken this approach.

The only tweak we've done that isn't already in libsawtooth is to change the errors to take ToString impl instead of String directly as it really makes writing things easier.

@suchapalaver
Copy link
Contributor Author

I'm not a fan of pulling in thiserror - errors are not difficult enough to warrant a dependency (despite us trying to make them more difficult by creating too many of them). That said, most the changes here have nothing to do with thiserror per-se, and I agree with the overall cleanups.

We should use the errors in libsawtooth (which were taken from splinter) - https://github.com/hyperledger/sawtooth-lib/tree/main/libsawtooth/src/error . Basically, InternalError, InvalidArgumentError, and InvalidStateError handle ~98% of all errors we ever need, with the 2% generally being compound errors containing 1 or more of those in an enum (and the occasional custom error when we actually need to return additional information inside the error). Probably 90%+ of everything in sawtooth would be InternalError - i.e. an error where the caller only knows something went wrong, but can't handle it in any specific way. This has worked extremely well in projects were we've taken this approach.

The only tweak we've done that isn't already in libsawtooth is to change the errors to take ToString impl instead of String directly as it really makes writing things easier.

Sawtooth already depends on thiserror transitively so I think you should consider going with the way I'm proposing, since it would offer improvements. We'd also be able to remove the dependency on fail if that sweetens it at all. Interested in what others think.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants