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

Add thiserror for better display error formatting #813

Closed
wants to merge 1 commit into from

Conversation

yukibtc
Copy link
Contributor

@yukibtc yukibtc commented Dec 7, 2022

Description

Resolve #555

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@yukibtc yukibtc closed this Dec 7, 2022
@yukibtc yukibtc deleted the addthiserror branch December 7, 2022 17:25
@yukibtc yukibtc restored the addthiserror branch December 8, 2022 09:14
@yukibtc yukibtc mentioned this pull request Dec 19, 2022
6 tasks
@yukibtc yukibtc reopened this Dec 21, 2022
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept ACK..

One extra change for from impls as we are using thiserror so lets use its full power..

src/blockchain/compact_filters/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Review ACK.. Changes looks good to me.. Would let others comment with their take on using thiserror as a dep..

@notmandatory
Copy link
Member

I'm in favor of this approach, but since it's adding a new dependency I think we should discuss also at our next bdk dev call, I've added it to the agenda.

@notmandatory
Copy link
Member

notmandatory commented Jan 3, 2023

@afilini in our call today we discussed making thiserror an optional dependency, but on second thought I don't think there's a reason to make it optional since it only adds macros to generate nice looking std::error::Error implementations, so doesn't change the bdk lib API and has no requirement to also use anyhow.

@danielabrozzoni
Copy link
Member

I don't think there's a reason to make it optional since it only adds macros to generate nice looking std::error::Error implementations

The reason for making it optional is that some projects want to cut dependencies to a minimum, and wouldn't use bdk if we include too many.

I agree with @afilini that it would be nice to have thiserror but behind a feature enabled by default

@yukibtc
Copy link
Contributor Author

yukibtc commented Jan 4, 2023

Add thiserror as optional dependency, in my opinion, make the code more messed up, less maintainable.
If you prefer to have fewer dependencies, it's better to avoid to add thiserror also as optional dependency and continue in the way of PR #814.
The result of PR #813 and #814 for the dev that use BDK library is the same, but for you the work it's doubled when you need to make some changes to error enums (using the approach of thiserror as optional feature).
Keep both approach it's a bad idea, in my opinion.

@notmandatory
Copy link
Member

notmandatory commented Jan 6, 2023

Add thiserror as optional dependency, in my opinion, make the code more messed up, less maintainable. If you prefer to have fewer dependencies, it's better to avoid to add thiserror also as optional dependency and continue in the way of PR #814. The result of PR #813 and #814 for the dev that use BDK library is the same, but for you the work it's doubled when you need to make some changes to error enums (using the approach of thiserror as optional feature). Keep both approach it's a bad idea, in my opinion.

I agree if we want to avoid making thiserror a required dependency it's better to just go with your approach in #814.

Reviewers, please check out #814, I've targeted it for the 0.27 release.

@yukibtc yukibtc closed this Feb 1, 2023
@yukibtc yukibtc deleted the addthiserror branch February 1, 2023 09:57
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.

Error formatting is poor
4 participants