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

refactor!: migrate from thiserror to snafu #181

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

lgiussan
Copy link
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
Contributor

Thanks for the PR!

@spaceandtimelabs spaceandtimelabs deleted a comment from Dustin-Ray Sep 26, 2024
Copy link
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.

crates/proof-of-sql/Cargo.toml Outdated Show resolved Hide resolved
tlovell-sxt
tlovell-sxt previously approved these changes Sep 26, 2024
Copy link
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
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 spaceandtimelabs:main Sep 27, 2024
10 checks passed
Copy link

🎉 This PR is included in version 0.25.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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