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.

Sign up for GitHub

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

Jump to bottom

Improve display error formatting #814

Merged
merged 1 commit into from Jan 30, 2023
Merged

Improve display error formatting #814

merged 1 commit into from Jan 30, 2023

Conversation

yukibtc
Copy link
Contributor

@yukibtc yukibtc commented Dec 7, 2022

Description

Closes #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

@rajarshimaitra
Copy link
Contributor

Alternate proposal.. Why not use thiserror in bdk to remove all the boiler plates around error structures? I think that will be a more valuable change across the lib than just implementing display.. There are various error conversions that can be simplified with it..

@afilini @notmandatory does that sound like a good approach to take? If yes, @yukibtc are willing to change this PR into that?

@yukibtc
Copy link
Contributor Author

yukibtc commented Dec 19, 2022 •
edited

I made both with thiserror and without. I used thiserror in PR #813
If it's better in that way, I'll close this PR and re-open #813

@rajarshimaitra
Copy link
Contributor

Personally that looks more cleaner to me..

@yukibtc
Copy link
Contributor Author

yukibtc commented Dec 21, 2022

Personally that looks more cleaner to me..

Yes, me too. I re-opened the PR #813.

@rajarshimaitra
Copy link
Contributor

For reviewers: This is competing to #813 ..

@yukibtc yukibtc mentioned this pull request Jan 4, 2023
Closed
6 tasks
@notmandatory notmandatory added the enhancement New feature or request label Jan 6, 2023
LLFourn
LLFourn reviewed Jan 9, 2023
View reviewed changes
src/error.rs Outdated
Self::Miniscript(err) => write!(f, "Miniscript error: {}", err),
Self::MiniscriptPsbt(err) => write!(f, "Miniscript PSBT error: {}", err),
Self::Bip32(err) => write!(f, "BIP32 error: {}", err),
Self::Secp256k1(err) => write!(f, "ECDSA error: {}", err),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this error is necessarily ECDSA?

@notmandatory notmandatory mentioned this pull request Jan 14, 2023
Closed
24 tasks
@notmandatory
Copy link
Member

notmandatory commented Jan 24, 2023 •
edited

@yukibtc this one should be ready to go, it just needs a rebase to update CI after #815 changed stable rust to 1.65. Thanks!

EDIT: hold off on rebasing until we decide on this PR: #842

@notmandatory
Copy link
Member

We've bumped MSRV to 1.57.0, please rebase.

notmandatory
notmandatory approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 9019793

@notmandatory notmandatory merged commit 6b92a16 into bitcoindevkit:master Jan 30, 2023
@yukibtc yukibtc deleted the displayerrorformatting branch January 30, 2023 19:38
@notmandatory notmandatory mentioned this pull request Feb 2, 2023
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LLFourn LLFourn LLFourn left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees

@yukibtc yukibtc

Labels
enhancement New feature or request
Projects
BDK Maintenance
Archived in project
Milestone
Release 0.27.0 Feature Freeze
Development

Successfully merging this pull request may close these issues.

Error formatting is poor
4 participants
@yukibtc @rajarshimaitra @notmandatory @LLFourn