Skip to content

refactor!: migrate from thiserror to snafu#181

Merged
JayWhite2357 merged 5 commits intospaceandtimefdn:mainfrom
zkVerify:refactor/snafu
Sep 27, 2024
Merged

refactor!: migrate from thiserror to snafu#181
JayWhite2357 merged 5 commits intospaceandtimefdn:mainfrom
zkVerify:refactor/snafu

Conversation

@lgiussan
Copy link
Copy Markdown
Contributor

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.

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

@JayWhite2357
Copy link
Copy Markdown
Contributor

Thanks for the PR!

@spaceandtimefdn spaceandtimefdn deleted a comment from Dustin-Ray Sep 26, 2024
Copy link
Copy Markdown
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/proof-of-sql/Cargo.toml Outdated
tlovell-sxt
tlovell-sxt previously approved these changes Sep 26, 2024
Copy link
Copy Markdown
Contributor

@tlovell-sxt tlovell-sxt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, LGTM - obviously Jay's suggestion will help with the failing CI.

JayWhite2357
JayWhite2357 previously approved these changes Sep 26, 2024
Copy link
Copy Markdown
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 JayWhite2357 merged commit 500cc7d into spaceandtimefdn:main Sep 27, 2024
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 0.25.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@lgiussan lgiussan deleted the refactor/snafu branch September 30, 2024 13:59
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.
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.

refactor: migrate from thiserror to snafu

3 participants