-
-
Notifications
You must be signed in to change notification settings - Fork 256
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
add support for bincode v2 #561
Conversation
Codecov Report
@@ Coverage Diff @@
## main #561 +/- ##
=======================================
- Coverage 95.7% 95.7% -0.0%
=======================================
Files 78 78
Lines 8703 8702 -1
=======================================
- Hits 8330 8329 -1
Misses 373 373
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What advantage does this give over using the native support for any type implementing |
Hey,
So here are the two points of why I want to use bincode;
That’s something else I wanted to talk about with you before merging anything. I think what I did is the worst way to implement Encode/Decode. tbh I did it because I needed it fast, but yes, it forces you to never change your structures (at least in a patch version), + it’s not optimal at all, I think. I don’t know much about the crate and dates in general, so let me know if I’m wrong, but I think we would be better off implementing our own Encode/Decode. Do you think storing a single |
Given that the linked issue is labelled as a bug, it seems reasonable to discount this as temporary.
Is this a technical restriction or merely unimplemented? If the latter, it's something that should be fixed upstream. With regard to deriving versus a manual implementation, a manual implementation would be preferred for the same reason it's preferred for serde. Upon a second glance at the diff, I noticed two things that I didn't initially:
Due to the uncertainties involved in if/when a full release will be created, in addition to the reasonable belief that the different serde attributes could be handled upstream, I'm going to go ahead and close this PR. |
That's a technical restriction, it can’t be implemented, and that’s the reason for all this bincodev2 thingy |
I'd likely need more of an explanation of the actual restriction if this were to be merged. However, the release candidate for ~18 months is more than reason to not merge this, at least for the time being. |
Hey @jhpratt ! I wanted to follow up with some specifics about the limitations of bincode+serde, just for clarification's sake (although I agree that it doesn't make sense to merge this until The main restrictions are around how bincode isn't a self-describing format, i.e. bincode can't serialize information about its own format. This means that it can't properly implement the This is obviously annoying when working with Hope that helps! |
The only time |
I think it would be less using #[derive(Serialize, Deserialize)]
#[serde(untagged)]
pub enum TimeOrU64 {
Time(Time),
U64(u64)
} which would serialize+deserialize fine in In this case, it would be nice to do something like this instead: #[derive(Serialize, Deserialize, bincode2::Encode, bincode2::Decode)]
#[serde(untagged)]
pub enum TimeOrU64 {
Time(Time),
U64(u64)
} Which would allow us to serialize/deserialize this struct using bincode as well as |
That doesn't strike me as a particularly strong use case. As |
Hey!
I don’t know if this is the kind of PR you would be willing to merge, but I would really like to use bincode in our workflow, and the only blocking crate is time, so I was wondering if it would be possible to add the support of bincode under a feature flag.