Skip to content

Serialize/Deserialize slices without length prefix#208

Open
alaadotsol wants to merge 5 commits intotafia:masterfrom
alaadotsol:master
Open

Serialize/Deserialize slices without length prefix#208
alaadotsol wants to merge 5 commits intotafia:masterfrom
alaadotsol:master

Conversation

@alaadotsol
Copy link
Copy Markdown

@alaadotsol alaadotsol 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

@alaadotsol alaadotsol changed the title Serialize/Deserialize functions without length prefix Serialize/Deserialize slices without length prefix Dec 30, 2021
@snproj
Copy link
Copy Markdown
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
Copy Markdown

Is there any progress on this pr :-)

@snproj
Copy link
Copy Markdown
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
Copy Markdown

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