Re-export SerDeError since it should be part of the API#438
Re-export SerDeError since it should be part of the API#438sagudev merged 1 commit intoservo:mainfrom
Conversation
| mod test; | ||
|
|
||
| pub use error::{IpcError, TryRecvError, TrySelectError}; | ||
| pub use error::{IpcError, SerDeError, TryRecvError, TrySelectError}; |
There was a problem hiding this comment.
I've been mulling this change over and there is a trade-off between strict API compatibility and usefulness of diagnostics. The change:
- Would cause API compatibility to be broken if Postcard is replaced by another serialisation mechanism in the future or if
postcard::Errorchanges incompatibly in the future. - Enables callers (i.e. other crates) to distinguish programmatically between various instances of
IpcError::SerializationError(I added a test to cover this).
There was a problem hiding this comment.
We should have exported this never the less.
There was a problem hiding this comment.
Actually given that this type is already used in public API it is surprising there is no rust/clippy warning.
glyn
changed the title
sagudev
left a comment
sagudev
left a comment
There was a problem hiding this comment.
We should export SerDeError, but as opaque error type, so on cannot really inspect inner error (we do not want postcard to be in out API) at least programmatically. For developers there is still Debug and Display which I think should be enough.
src/error.rs
Outdated
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn distinguish_between_serialization_errors() { | ||
| let err = IpcError::SerializationError(SerDeError(postcard::Error::DeserializeBadBool)); | ||
| if let IpcError::SerializationError(ser_de_err) = err { | ||
| assert_eq!(ser_de_err.0, postcard::Error::DeserializeBadBool); | ||
| } else { | ||
| panic!("Unexpected enum variant") | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn display_ser_de_error() { | ||
| let ser_de_error = SerDeError(postcard::Error::DeserializeBadBool); | ||
| assert_eq!( | ||
| format!("{}", ser_de_error), | ||
| "Serialization/Deserialization: Found a bool that wasn't 0 or 1" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think the only reason why distinguish_between_serialization_errors works is because these are unit tests, thus they live in same crate, thus they can inspect inner of pub struct SerDeError(#[from] pub(crate) postcard::Error);, but they should not work as integration tests because one cannot access inner content.
There was a problem hiding this comment.
Correct - I tried it and it didn't work.
| mod test; | ||
|
|
||
| pub use error::{IpcError, TryRecvError, TrySelectError}; | ||
| pub use error::{IpcError, SerDeError, TryRecvError, TrySelectError}; |
There was a problem hiding this comment.
We should have exported this never the less.
glyn
force-pushed
the
reexport-SerDeError
branch
from
5259374 to
f5502ea
Compare
|
I agree with sagudev that the error can be exposes as long as we do not expose the inner error so we can easily change it without breaking api compatibility. |
|
@sagudev @Narfinger I agree with your reasoning, so I've cut this PR back to the initial commit - the re-export of SerDeError. |
3 participants
Also display the detail of a SerDeError.
@Narfinger: please review.