-
Notifications
You must be signed in to change notification settings - Fork 930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: properly encapsulate quick_protobuf::Error
#3894
Conversation
These leak into the public API.
pub struct DecodeError(String); | ||
|
||
impl From<quick_protobuf::Error> for DecodeError { | ||
fn from(e: quick_protobuf::Error) -> Self { | ||
Self(e.to_string()) | ||
} | ||
} | ||
|
||
impl fmt::Display for DecodeError { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
write!(f, "{}", self.0) | ||
} | ||
} | ||
#[error(transparent)] | ||
pub struct DecodeError(quick_protobuf::Error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused. Isn't the constructor of DecodeError
the same as a From<quick_protobuf::Error>
? Asked in a different way, changing DecodeError(quic_protobuf::Error)
to e.g. DecodeError(SomeNewType)
a breaking change? (Where SomeNewType
might as well be quic_protobuf
just as a new version.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if the field is crate-private, you can't construct it from outside this crate. Trait impls are global and because From
can be imported and used by anyone, anyone can call this implemented and thus depend on the underlying quick_protobuf
version.
It is a very nasty situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks for explaining!
Description
I noticed that we also still leak that dependency in several crates by providing a
From
impl so I've removed that one as well.Resolves #3534.
Notes & open questions
Change checklist