-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fix: Split serde bound for RaftError
into serialize and deserialize
#991
Conversation
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.
Oh thank you! I've not considered such subtle scenario yet.
FWIW, I was confused by the presence of this bound, so I looked into the way this concrete derivation works. I think the problem is not the bound on the #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
+#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
-#[cfg_attr(
- feature = "serde",
- derive(serde::Deserialize, serde::Serialize),
- serde(bound(serialize = "E: serde::Serialize")),
- serde(bound(deserialize = "E: for <'d> serde::Deserialize<'d>"))
-)]
pub enum RaftError<NID, E = Infallible>
where NID: NodeId
{
#[error(transparent)]
APIError(E),
+ #[serde(bound = "")]
#[error(transparent)]
Fatal(#[from] Fatal<NID>),
} |
The problem is not the bound on the `E` generic, but rather, the fact that `NID` is receiving two different `Serialize/Deserialize` bounds; one from the derive annotation, and one from the `NID: NodeId` trait bound. This modification is from suggestions by @tvsfs about the serde bound: databendlabs#991 (comment)
Looks good. Let me check it in this PR: |
The problem is not the bound on the `E` generic, but rather, the fact that `NID` is receiving two different `Serialize/Deserialize` bounds; one from the derive annotation, and one from the `NID: NodeId` trait bound. This modification is from suggestions by @tvsfs about the serde bound: databendlabs#991 (comment)
The problem is not the bound on the `E` generic, but rather, the fact that `NID` is receiving two different `Serialize/Deserialize` bounds; one from the derive annotation, and one from the `NID: NodeId` trait bound. This modification is from suggestions by @tvsfx about the serde bound: databendlabs#991 (comment)
The problem is not the bound on the `E` generic, but rather, the fact that `NID` is receiving two different `Serialize/Deserialize` bounds; one from the derive annotation, and one from the `NID: NodeId` trait bound. This modification is from suggestions by @tvsfx about the serde bound: #991 (comment)
While implementing a struct that has a generic
RaftError<E>
as a field, I ran into some issues with trait bounds becauseRaftError
does not make a distinction between the serdeSerialize
andDeserialize
bounds. The consequence is that anyimpl
block that requires my struct to beSerialize
will requireE
to both beSerialize
andDeserializeOwned
, which is not necessary. This PR splits the two bounds.Checklist
This change is