Migration from io::Error to io::ErrrorKind #560
Conversation
dr-orlovsky
added
the
API break
|
NACK. I have tried this before myself and I gave up on it. Needed those types on error types usually indicates some bad practice. Also, giving up |
|
NACK cee0a54 Removing the impl of |
|
Agree that However will argue that using |
There was a problem hiding this comment.
However will argue that using io::Error instead of io::ErrorKind at expense of having errors non-clonable, non-equatable, non-orderable and non-hashable is a bad practice. There a big discussion in rust community going on regarding the io::Error being bad practice and probably @Kixunil has some strong arguments about it.
I am happy to hear more arguments for this! In my current experience, errors shouldn't usually need be clonable etc and I am worried that removing the inner IO error from cause for example makes use lose viable information.
src/util/bip32.rs
Outdated
| @@ -589,7 +586,7 @@ impl ExtendedPrivKey { | |||
| network: network, | |||
| key: secp256k1::SecretKey::from_slice( | |||
| &data[46..78] | |||
| ).map_err(Error::Ecdsa)?, | |||
| ).map_err(Error::Secp256k1)?, | |||
There was a problem hiding this comment.
Given that we have a From<secp256k1::Error> for Error implementation, I think this map_err is redundant because ? calls Error::from for you.
| @@ -212,9 +212,7 @@ impl Map for Global { | |||
| entry.insert((fingerprint1, derivation1)); | |||
| continue | |||
| } | |||
| return Err(psbt::Error::MergeConflict(format!( | |||
| "global xpub {} has inconsistent key sources", xpub | |||
There was a problem hiding this comment.
Not sure if it is intentional but we lost this part of the error message.
There was a problem hiding this comment.
We do not: we have a dedicated error type which may happen only for a global xpub (and we embed xpub into error)
| fn cause(&self) -> Option<&dyn error::Error> { | ||
| match *self { | ||
| Error::Io(ref e) => Some(e), | ||
| Error::Psbt(ref e) => Some(e), |
There was a problem hiding this comment.
psbt::Error is still a valid cause, I think we should keep that!
|
Indeed, as I said The main reason for my criticism of It is possible for a motivated programmer to wrap IO errors so that they would contain the path. (I do this often, creating my own error type.) This is not translatable and not While I would love to see Rust crates to move away from So, it's NACK from me as it's written now. Regarding
I know of at least one case when it does make sense to make errors clonable: logging into multiple sinks or logging (on another thread) and simultaneously sending a response. I actually was in a situation where this was needed. Although I managed to workaround lack of cloning, I can imagine a situation where such warkaround isn't impossible. Errors should be pretty much PODs, maybe boxed, but still PODs. But as I explained above, this PR has serious downsides. |
dr-orlovsky
changed the title
|
Closed. All non-controversial parts are moved to #625 |
I do agree to do this, however, the guidelines recommend it only if Display isn't already printing the wrapped error rust-lang/project-error-handling#27 (comment) |
|
@RCasatta yes, we need to change both. :) |
994079b Refactoring error variants: removing unused; better names & inner types (Dr Maxim Orlovsky) Pull request description: Removes controversial aspects from #560 (all `io::Error`-related changes) and leaves the rest ACKs for top commit: sanket1729: ACK 994079b apoelstra: ACK 994079b Tree-SHA512: 020e49193c885e862f45e5f7baabf1d22a3ec09e78fd7f573b2f3d327beb4f91683951ba080b3d804e8337a188dcad0f38ba70ee8059aef0681a0b2bba0a2140
This completes Error derives work from #558 and #559 with API-breaking changes, including
io::Errortoio::ErrorKindtype, which isCopy,Eq,Ord&Hashand is easier to handle. The cost of this is the need to get rid of deprecatedError::causeimplementations, which I think is also beneficial (and at least one of Error types hadn't it anyway)