Introduce dedicated error types to the lapin-futures crate #145
Conversation
futures/examples/consumers.rs
Outdated
| }).and_then(|(client, heartbeat)| { | ||
| tokio::spawn(heartbeat.map_err(|e| eprintln!("heartbeat error: {:?}", e))) | ||
| .into_future().map(|_| client).map_err(|_| io::Error::new(io::ErrorKind::Other, "spawn error")) | ||
| .into_future().map(|_| client).map_err(|_| err_msg("spawn error")) |
There was a problem hiding this comment.
What's the difference between err_msg and Error::new? I see the two being used in this patch
There was a problem hiding this comment.
lapin_futures::Error can only be created from within the lapin_futures crate, by using the lapin_futures::Error::new function. This means that it can't be called in the examples.
err_msg is a function provided by the failure crate, creating a new failure::Error from a Displayable item.
futures/examples/client.rs
Outdated
| @@ -82,7 +84,7 @@ fn main() { | |||
| c.basic_ack(message.delivery_tag, false) | |||
| }) | |||
| }) | |||
| }) | |||
| }).map_err(Error::from) | |||
| }).map_err(|err| eprintln!("error: {:?}", err)) | |||
There was a problem hiding this comment.
From what I understand, we could/should drop the :? in places like that, right?
There was a problem hiding this comment.
According the the failure::Fail trait documentation, the Debug format is supposed to be "a verbose, developer-focused representation of the error", whereas the Display format is supposed to be "a user-friendly representation of the error", so it would make sense to switch from {:?} to {}, especially in the examples.
I'll change these in my next batch of commits.
futures/src/transport.rs
Outdated
| let (consumed, f) = match parse_frame(buf) { | ||
| Err(e) => { | ||
| if e.is_incomplete() { | ||
| return Ok(None); | ||
| } else { | ||
| return Err(io::Error::new(io::ErrorKind::Other, format!("parse error: {:?}", e))); | ||
| return Err(CodecError::ParseError); |
There was a problem hiding this comment.
Don't we lose the error there?
There was a problem hiding this comment.
My bad, I'll add proper causes to the ParseError and GenerationError variants.
This change replaces all occurrences of io::Error by the newly created Error type, the only exception being for AMQPCodec. This change allows future-proofing the error that can be returned by the crate. For example by changing the internals of the Error type, it is now possible to provide users with a way of knowing what kind of error occurred without having to resort to parsing error messages. The only exception to this change concerns AMQPCodec: a new Error type was created just for it because tokio-codec's Encoder and Decoder traits impose a From<std::io::Error> bound of the error type. Because I didn't want to introduce a way to create an Error instance from outside the crate, I isolated the possible errors from this section into its own type.
Errors are now printed using the `Display` formatter instead of the `Debug` formatter.
kureuil
force-pushed
the
introduce-error-type
branch
from
da50118
to
3148e07
Compare
|
@Keruspe rebased onto current master, awaiting for your review :) |
|
Thanks a lot for this. Always wanted to give failure a look and never had the occasion before |
This change replaces all occurrences of
io::Errorby the newly createdErrortype, the only exception being forAMQPCodec. This change allowsfuture-proofing the error that can be returned by the crate. For example
by changing the internals of the
Errortype, it is now possible toprovide users with a way of knowing what kind of error occurred without
having to resort to parsing error messages.
The only exception to this change concerns
AMQPCodec: a newCodecErrortypewas created just for it because
tokio-codec'sEncoderandDecodertraitsimpose a
From<std::io::Error>bound of the error type. Because I didn'twant to introduce a way to create an
Errorinstance from outside thecrate, I isolated the possible errors from this section into its own
type.