fix(transport): Improve Error type
#217
Conversation
| } else { | ||
| write!(f, "{}", self.kind) | ||
| } | ||
| self.0.fmt(f) |
There was a problem hiding this comment.
Would it make sense to add something here related to this being a transport error? Since basically we are wrapping any boxed error it might be nice to ensure that this type is know to be coming from the transport layer?
There was a problem hiding this comment.
That breaks entire point of the error just being a proxy for the underlying error.
This error is only ever returned back from the transport module. So any user must already know it's a transport error. No?
There was a problem hiding this comment.
Well, I guess you can use the type system to check what error this. looks like this is what hyper does for debug https:/hyperium/hyper/blob/master/src/error.rs#L322
and it seems it does what you did here for display too.
There was a problem hiding this comment.
I'm really not a fan of how hyper designed their errors :)
There was a problem hiding this comment.
Can you explain why? I personally have not had an issue with it but not sure what others have experienced either :)
There was a problem hiding this comment.
No you can't really use the typesystem for anything with any of these opaque errors. All their Kind/ErrorKind enums are private :(
There was a problem hiding this comment.
The only thing they ever allow you to do currently is to print them. And even then they don't look good because they have a ton of duplicated info in each layer.
There was a problem hiding this comment.
You should be able to downcast them https://docs.rs/hyper/0.13.1/hyper/error/struct.Error.html#method.into_cause for std errors and h2 but beyond that yeah its a bit tough, the problem is adding more error types becomes a breaking change. Now that we have the non exhaustive it should be better but I guess rusts error handling is still a bit in flux. Do you have an error type that you like as a good example?
There was a problem hiding this comment.
I think we have currently, with this PR, made it impossible to downcast the error to an underlying hyper error. Our new tonic error can't be downcasted because it's a newtype wrapping the hyper error. And calling source() on it will not yield the hyper error but rather the source of it.
That being said, I have personally never downcasted an error nor felt the need to do so.
No I don't have an example of a well written error type. I do know of some concrete pain points in, what I consider, bad errors. But I don't have a manual for nice ones. If I had I would contribute it to the official API guidelines, a document I really think is lacking in the error area.
If you make it possible to destructure the error, it's probably correct to break the API when new errors are added? Because if you have such errors you tell the user what can happen and encourage them to handle the different error cases. Which means your API indeed changes if you add a new error. Users should then get compilation errors because there will be new types of errors they are expected to handle.
|
This looks great! Thank you! Looks like you just need to run |
faern
force-pushed
the
fallthrough-error
branch
from
bd20dbb
to
858d655
Compare
LucioFranco
changed the title
Error type
Motivation
Fixes #213
Solution
Makes the
tonic::transport::Errortype much simpler. It just forwards what's in the source error now. It was previously opaque anyway so a user of the library could not match on theErrorKindor anything like that anyway.However, this just solves the problem of the error not having redundant information in it. The library still uses a lot of opaque
Box<dyn Error>internally. I personally think it would be nicer iftonicexposed a properhyper::Errorwhere the error comes fromhyperetc. But I did not feel that was in scope for getting this part done.