Skip to content
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

Serialize/Deserialize slices without length prefix #208

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AlaaZorkane
Copy link

@AlaaZorkane AlaaZorkane commented Dec 30, 2021

Following: #202

This PR adds the following:

  • Message writer without prefixing a length
  • Slice serializer without prefixing a length
  • Reading messages that aren't prefixed with a length
  • Reading slices that aren't prefixed with a length
  • Read/Write test for all the above implementations
  • Return prefixed length on serialization method

@AlaaZorkane AlaaZorkane changed the title Serialize/Deserialize functions without length prefix Serialize/Deserialize slices without length prefix Dec 30, 2021
@snproj
Copy link
Collaborator

snproj commented Dec 21, 2022

Hi! The PR looks good; I was actually considering a few options, since this seems to be a common stumbling block for new users:

  • Modify serialize_into_slice() and deserialize_from_slice() to no longer include the length prefix
  • Modify serialize_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)
  • Keep serialize_into_slice() and deserialize_from_slice() the same, but add documentation
  • Keep serialize_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.

@ScarboroughCoral
Copy link

Is there any progress on this pr :-)

@snproj
Copy link
Collaborator

snproj commented Jan 6, 2023

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!

@ScarboroughCoral
Copy link

I think can add two methods this pr or next pr

  1. serialize_into_slice_with_variant(variant: Option<T>, msg: M)
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants