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

Consider changing marshal default to not length prefix #219

Open
ebuchman opened this issue Aug 14, 2018 · 4 comments
Open

Consider changing marshal default to not length prefix #219

ebuchman opened this issue Aug 14, 2018 · 4 comments

Comments

@ebuchman
Copy link
Contributor

Currently we have eg. MarshalBinary and MarshalBinaryBare where the former includes a length prefix and the later does not. It seems more natural to switch the default behaviour to not be length prefixed (afaik proto3 does not auto-include a length prefix), and to have MarshalBinaryLengthPrefix instead of MarshalBinaryBare

@liamsi
Copy link
Contributor

liamsi commented Aug 28, 2018

I agree, this should be the default behaviour. And you are right, that proto3 doesn't encode the length by default. Length delimiting is used for streaming AFAIK. I think some implementations do not even provide length delimited encoding / decoding. Most do though (java, prost/rust).

I think we should change this pre-launch as this would be breaking. Do you agree?

@ebuchman
Copy link
Contributor Author

ebuchman commented Aug 28, 2018

Yes I do! Should get confirmation from @jaekwon too though

@jaekwon
Copy link
Contributor

jaekwon commented Aug 30, 2018

yes :)

@liamsi
Copy link
Contributor

liamsi commented Oct 15, 2018

First step (rename MarshalBinary) was addressed in #222. Will update depending projects to use the new API. After that we could either rename MarshalBinaryBare to MarshalBinary, or, we could keep the Bare postfix.

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

No branches or pull requests

3 participants