fix(autonat::v2::client): DialBackError visibility
#6168
Conversation
| pub struct Error { | ||
| pub(crate) inner: dial_request::DialBackError, | ||
| } | ||
|
|
||
| impl Display for Error { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| Display::fmt(&self.inner, f) | ||
| } | ||
| } | ||
|
|
||
| impl Debug for Error { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| Debug::fmt(&self.inner, f) | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldnt it be better to leave this in place but implement Error (or maybe use thiserror::Error and use #[error(transparent)]) so you can downcast to DialBackError?
There was a problem hiding this comment.
I honestly gave that a small thought initially. I definitely like the second approach more. But I fail to identify a benefit from that layer of indirection, so came with this approach; do you have some on your mind? Like IIRC the idea of thiserror is that someone downstream who uses it can wrap this easily for their needs, and we don't imply it on those who don't want to use it. 😆
There was a problem hiding this comment.
yup, I agree with Darius I don't think this needs to be a breaking change, we can probably implement downcast(&self) -> DialBackError wdyt?
There was a problem hiding this comment.
@jxs
It's hard for me to think about this thoroughly as I'm lacking so much of the wider context. I only have the thought that if it lives in pub mod v2 then this doesn't seem to be a big deal to straighten it in a breaking manner. Like it will anyway either be replaced with v3 or become autonat::client sooner or later.
There was a problem hiding this comment.
As DialBackError already has thiserror::Error on it I'd argue the change is not even breaking, as all downstream could do is match Result::Err, Display or format it -- a private field prevented anything else hence everything would continue to work after this.
Tbh, I'd rather see here Result<Result> for more obvious distinguishing networking failure and autonat success. But that would be breaking.
Co-authored-by: Darius Clark <dariusc93@users.noreply.github.com>
3 participants
Description
Fix visibility so downstream could differentiate between
NoConnectionandStreamFailed.Notes & open questions
I guess this one doesn't need adding a test, but if you suggest a good one that would be interesting. Though I can imagine you'd like to add this somewhere in the integration tests on the whole
libp2plevel in a separate issue.Change checklist