fix: no Debug on Display implementations
#2083
Conversation
Replace Debug-based formatting with user-friendly Display messages. Add helper to shorten descriptors for concise error output.
Limited to 3 max shown items and added a suffix when there are additional entries.
Format magic bytes as 0x-prefixed hex.
Dmenec
force-pushed
the
fix/no-debug-on-display-impls
branch
from
492114b to
b73016e
Compare
|
Concept ACK — this approach looks good, thanks for the changes. I ran the tests locally and they all passed. While trying to recreate the initial issue’s scenario (#1933), I found that I have a few small suggestions below. Most of these are nits, but since this PR is already addressing the area, I thought it’d be reasonable to mention them here:
|
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Quality work.
Just a couple requested changes:
- Commit messages should contain a scope (as mentioned in the conventional commits spec). The scope should be the name of the package affected:
core,chain,example, etc. - Commits that break the API should be marked with
!.
And a nit:
- Error messages should be somewhat consistent. I personally don't think we should suggest actions in the message, just state what the error is.
| write!( | ||
| f, | ||
| "attempt to re-assign descriptor {descriptor:?} already assigned to {existing:?}" | ||
| "Descriptor '{}' is already in use by keychain '{}'. Please use a different descriptor.", |
There was a problem hiding this comment.
Nit
| "Descriptor '{}' is already in use by keychain '{}'. Please use a different descriptor.", | |
| "descriptor '{}' is already in use by another keychain '{}'", |
| write!( | ||
| f, | ||
| "attempt to re-assign keychain {keychain:?} already assigned to {existing:?}" | ||
| "Keychain '{}' is already associated with descriptor '{}'. Please choose a different keychain.", |
There was a problem hiding this comment.
Nit
| "Keychain '{}' is already associated with descriptor '{}'. Please choose a different keychain.", | |
| "keychain '{}' is already associated with another descriptor '{}'", |
|
|
||
| #[cfg(feature = "std")] | ||
| impl<K: core::fmt::Debug> std::error::Error for InsertDescriptorError<K> {} | ||
| impl<K: core::fmt::Display + core::fmt::Debug> std::error::Error for InsertDescriptorError<K> {} |
There was a problem hiding this comment.
Technically this is a breaking change, so it should be reflected in the commit message (!).
4 participants
Description
Fixes #1933, remove the usage of
DebugonDisplayimplemetations mentioned in #1881 (comment)Changelog notice
Debugusage withDisplaymessages forInsertDescriptorError,CalculateFeeErrorandStoreError.InvalidMagicBytesnow displays expected and actual bytes in hexadecimal instead of usingDebug.short_descriptorfunction to shorten descriptors for concise error output.SyncRequestDisplayfor Esplora and Electrum examples.Checklists
All Submissions:
just check,just fmtandjust testbefore committing