-
Notifications
You must be signed in to change notification settings - Fork 301
Expose types implementing serde::Serializer
and Deserializer
#729
Conversation
e0cf3e6
to
f286ab3
Compare
After some experimentation, I decided to not use The PR exposes two new entities: |
Sorry for the late response, the last month has been very busy. We're still thinking on if we want this change in bincode or not. Thank you for your patience 🙏 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Bump, sorry life has been busy |
We are considering making a bincode-utils or -contrib crate. If we do this, do you think option 4 would be best? |
Option 4 would be best for that kind of crate, but it will still require the main library to expose something that implements Edit: this PR actually just exposes |
I merged our trunk into your branch but it seems like there's a CI issue, once that's resolved I think this is good to merge! |
1ac069b
to
938ad60
Compare
Rebased to trunk, the tests pass for me locally |
No idea why I can't reproduce it locally, but I think this should fix it. |
Thanks! |
This is sort of a proof-of-concept PR, I would like your input on it even though it's still in draft.
The goal here is to expose types implementing
serde::Serializer
andDeserializer
so thatbincode
could be used with https://docs.rs/erased-serde.Serializer
is straightforward, butDeserializer
can be exposed in two ways:D
, wherefor<'a> &'a mut D: Deserializer<'de>
. This is done e.g. inpostcard
. The problem here is that using such types generically involves passing around an ugly bound on lifetimes, which cannot be encapsulated in a trait. Also, dealing with such types inerased_serde
is somewhat complicated (albeit possible).D: Deserializer<'de>
. The downside of this is that in the format implementations (likebincode
)serde::Deserializer
is generally implemented for&mut Something
, or a wrapper of it (SerdeDecoder
inbincode
's case), because it has to be passed to nested list/struct/etc deserializers. So creating another implementation forD
involves writing down the massive trait impl one more time.So, I would like your input on the way to proceed with this. I see the following options:
bincode
at all. Feel free to close the PR then.D
, wherefor<'a> &'a mut D: Deserializer<'de>
. This requires minimum changes, but creates problems for the users, as described above.D: Deserializer<'de>
, writing down anotherDeserializer
impl (of which there are already two in the codebase).D: Deserializer<'de>
, relying on a helper crate that I made (https://docs.rs/serde-persistent-deserializer), which is implemented in this PR. The reason I put it there is that I want to create a similar PR for other formats, and I would like to avoid writingDeserializer
impls in each of them. I understand if you want to minimize the number of dependencies, then the contents of that crate can be brought in verbatim.(This PR currently does not expose
Serializer
, but that's easy, and I'll add it later if you decide not to go with Option 1).A few things about the PR itself.
Deserializer
impl inde_borrowed
, and it's almost possible by just replacingde_borrowed::SerdeDecoder
withde_owned::SerdeDecoder
, but a single test fails because these impls are slightly different. I am not sure if it's possible to merge them.de_owned::decode_from_slice()
("Note that this does not work with borrowed types like&str
or&[u8]
. For that use [borrow_decode_from_slice].") Seems to be working fine, as I can just callde_borrowed::borrow_decode_from_slice()
from it.