Use dedicated error type for TryInto errors #220
Conversation
JelteF
force-pushed
the
error-type-for-try-into
branch
from
53df546 to
27c50fe
Compare
src/errors.rs
Outdated
| @@ -0,0 +1,32 @@ | |||
| use core::fmt; | |||
|
|
|||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Open question 1: Do we to derive Debug? Maybe not, because it requires all types for which TryFrom is derived to also have a Debug implementation. Can we make this conditionally derive Debug only when T implements Debug itself?
Open question 2: Do we want to implement the Error trait for this error type? This doesn't work for no_std currently, but we could make std an optional default enabled feature.
Edit: Actually it's probably fine to require a Debug implementation, if people complain about it we can reconsider. Having the error type implement Debug is also a prerequisite for implementing the Error trait.
There was a problem hiding this comment.
Open question 1: Do we to derive
Debug?
Yes. We do want to derive Debug (and Clone too), because it's so unpleasant situation when third-party lib doesn't provide a Debug impl for a type, and you should squat-around it with wrappers in case you just want to put dbg!(), and I hit it so many times 🤬
Maybe not, because it requires all types for which TryFrom is derived to also have a Debug implementation. Can we make this conditionally derive Debug only when T implements Debug itself?
Yup. Event standard #[derive(Debug)] will generate a proper impl<T: Debug> Debug for MyError<T> in this situation. We don't need to do something additional here.
Open question 2: Do we want to implement the Error trait for this error type? This doesn't work for no_std currently, but we could make std an optional default enabled feature.
Yes. Definitely.
There was a problem hiding this comment.
Added deriving Clone and Copy in a6e68df. These are conditional in the same way as #[derive(Debug)].
There was a problem hiding this comment.
Yup. Event standard #[derive(Debug)] will generate a proper impl<T: Debug> Debug for MyError in this situation. We don't need to do something additional here.
Yes of course, it seems I was misreading the following error message when removing Debug from MixedData:
error[E0277]: `MixedData` doesn't implement `Debug`
--> src/lib.rs:250:42
|
14 | assert_eq!(123u32, mixed_int1.try_into().unwrap());
| ^^^^^^ `MixedData` cannot be formatted using `{:?}`
|
= help: the trait `Debug` is not implemented for `MixedData`
= note: add `#[derive(Debug)]` to `MixedData` or manually `impl Debug for MixedData`
= help: the trait `Debug` is implemented for `TryIntoError<T>`
= note: required because of the requirements on the impl of `Debug` for `TryIntoError<MixedData>`
the reason it's complaining here is because unwrap requires an E that implements Debug
src/errors.rs
Outdated
| } | ||
|
|
||
| impl<T> TryIntoError<T> { | ||
| pub fn new( |
| use core::fmt; | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct TryIntoError<T> { |
There was a problem hiding this comment.
I'd like to add docs for this error type as it's part of public API now.
| @@ -32,3 +32,6 @@ | |||
|
|
|||
| #[doc(inline)] | |||
| pub use derive_more_impl::*; | |||
|
|
|||
| mod errors; | |||
| pub use crate::errors::TryIntoError; | |||
There was a problem hiding this comment.
Also, I think this should be feature-gated by "try_into".
| @@ -75,6 +75,25 @@ jobs: | |||
|
|
|||
| - run: cargo test --workspace --features testing-helpers | |||
|
|
|||
| no_std: | |||
There was a problem hiding this comment.
@JelteF I've added this CI check, so we can track possible no_std regressions on CI.
There was a problem hiding this comment.
I'm supprised it works, because this issue looks like it would impact this crate: hobofan/cargo-nono#47
There was a problem hiding this comment.
@JelteF the reason is explained:
there is just no logic in place yet to track and evaluate compiler switches on individual use items (as evaluating arbitrary compiler switches was a rabbit hole I didn't want to go into).
We just have no compiler switches on use statements at the moment. So, we're lucky.
Instead of returning a string as the error, this returns a dedicated
error type. This has the big benefit that in case of an error it's
possible to get back the original value by using
error.input.Previously the original value would be dropped in case of an error.
Fixes #173
Fixes #130