Open AlaaZorkane opened 2 years ago
Hi! The PR looks good; I was actually considering a few options, since this seems to be a common stumbling block for new users:
serialize_into_slice()
and deserialize_from_slice()
to no longer include the length prefixserialize_into_slice()
and deserialize_from_slice()
to no longer include the length prefix, AND add two new methods that keep the old behavior (still a breaking change)serialize_into_slice()
and deserialize_from_slice()
the same, but add documentationserialize_into_slice()
and deserialize_from_slice()
the same, but add documentation AND two new methods that follow expectations better (basically this PR)What do you think? Something to consider is the fact that we might have breaking changes already anyway from other PRs #235 and #240.
Is there any progress on this pr :-)
In my opinion, since we already have breaking changes from other PRs that look like they're going to be merged, let's go with option 1 above. That is, just change the functions to no longer include the prefix, and don't include any backwards-compatibility functions that do, because that seems a bit messy to me. Counterarguments are welcome though!
I don't know of any particular use for the functions that have length prefixes, so my instinct is to remove them for the sake of keeping the API intuitive and small. Are you okay with this @AlaaZorkane?
I understand of course that this PR was awhile ago haha 😅 so I will likely make the change myself in a bit if there's no response. Do let me know if you want to do it yourself though!
I think can add two methods this pr or next pr
serialize_into_slice_with_variant(variant: Option<T>, msg: M)
deserialize_from_slice with_variant() -> Result<variant:Option<T>>
because I don't know what kind of a binary message is, I can serialize the message with a variant at head, and deserialize the message getting the variant and check my variant table get the type of the binary message and deserialize it.
Following: https://github.com/tafia/quick-protobuf/issues/202
This PR adds the following: