refactor!: migrate from thiserror to snafu#181
Merged
JayWhite2357 merged 5 commits intospaceandtimefdn:mainfrom Sep 27, 2024
Merged
refactor!: migrate from thiserror to snafu#181JayWhite2357 merged 5 commits intospaceandtimefdn:mainfrom
thiserror to snafu#181JayWhite2357 merged 5 commits intospaceandtimefdn:mainfrom
Conversation
Contributor
|
Thanks for the PR! |
JayWhite2357
requested changes
Sep 26, 2024
Contributor
JayWhite2357
left a comment
There was a problem hiding this comment.
Thanks for the big effort on this! I appreciate the attention to detail.
One small change about the std feature. I think the solution is to simply drop the last commit, and it will be good. I'll add an equivalent change in my next 1 or 2 PRs.
tlovell-sxt
previously approved these changes
Sep 26, 2024
Contributor
tlovell-sxt
left a comment
There was a problem hiding this comment.
This is great, LGTM - obviously Jay's suggestion will help with the failing CI.
JayWhite2357
previously approved these changes
Sep 26, 2024
Contributor
JayWhite2357
left a comment
There was a problem hiding this comment.
LGTM.
Looks like you need to rebase and resolve some conflict with main. I'll need to re-approve once you do that, but it should be good once you do.
JayWhite2357
approved these changes
Sep 27, 2024
tlovell-sxt
approved these changes
Sep 27, 2024
|
🎉 This PR is included in version 0.25.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
JayWhite2357
pushed a commit
that referenced
this pull request
Mar 7, 2025
# Rationale for this change In order to make this rep `no_std` compatible, we need `no_std` compatible errors. `thiserror` is not `no_std` compatible, while `snafu` is. We are currently using `thiserror`. Note: While dtolnay/thiserror#304 is an open PR, it is not clear how soon this will get merged. Additionally, Substrate uses `snafu`, which would help unify dependencies for Substrate-based integrations. # What changes are included in this PR? NOTE: the individual commits may be good to review individually. - All error enum variants consisting of tuple structs are transformed into named structs. This is necessary because `snafu` does not support tuple structs. - Every `#[derive(Error)]` is substituted with the analogous `#[derive(Snafu)]`. In particular: * `#[error(...)]` attributes are substituted with equivalent `#[snafu(display(...))]` attributes * `#[error(transparent)]` attributes are substituted with equivalent `#[snafu(transparent)]` attributes (which also derive the corresponding `From` implementation) * For `ConversionError::TimestampConversionError`, the `#[snafu(context(false), display(...))]` attribute is used for deriving a `From` implementation, and at the same time maintain the custom error message - A `std` feature is introduced for the `proof-of-sql` crate, which in turns activates the `snafu/std` feature. The `std` feature is required for the `posql_db` example, because `Clap` relies on the `std::error::Error` trait. - `thiserror` still appears in the dependency tree (`cargo tree -i thiserror`), but only as a transitive dependency (via `blitzar`, and dev-dependencies) # Are these changes tested? Yes. This PR is a refactoring, and all existing tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This PR should close #174.
What changes are included in this PR?
NOTE: the individual commits may be good to review individually.
snafudoes not support tuple structs.#[derive(Error)]is substituted with the analogous#[derive(Snafu)]. In particular:#[error(...)]attributes are substituted with equivalent#[snafu(display(...))]attributes#[error(transparent)]attributes are substituted with equivalent#[snafu(transparent)]attributes (which also derive the correspondingFromimplementation)ConversionError::TimestampConversionError, the#[snafu(context(false), display(...))]attribute is used for deriving aFromimplementation, and at the same time maintain the custom error messagestdfeature is introduced for theproof-of-sqlcrate, which in turns activates thesnafu/stdfeature. Thestdfeature is required for theposql_dbexample, becauseClaprelies on thestd::error::Errortrait.thiserrorstill appears in the dependency tree (cargo tree -i thiserror), but only as a transitive dependency (viablitzar, and dev-dependencies)Are these changes tested?
Yes. This PR is a refactoring, and all existing tests pass.