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

pure kotlin dependency-free Marshaller/Unmarshaller and native #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phcoder
Copy link

@phcoder phcoder commented Jan 27, 2019

This provides native variants for all the components.

Code is adapted from java protobuf code.

@phcoder phcoder mentioned this pull request Jan 27, 2019
@cretz
Copy link
Owner

cretz commented Jan 28, 2019

That's a pretty significant level of effort, thanks. Notes:

  1. I think at a higher level saying you referenced the Google protobuf lib Java reference impl is reasonable over embedding the copyright headers.
  2. I don't think we should remove the common Marshaller, Unmarshaller, and Sizer. I don't want the "pure" impl to take over the other ones, just be an option instead of them. Also, I would like backwards compatibility (at least mostly).
  3. Same with Util...some platforms have faster ways of doing this, I don't want to force the pure Kotlin one on those platforms.
  4. Do you mind if instead of accepting this PR wholesale, if I work some of it into a new PR that takes a lighter approach? I will gladly give you credit in the README for the initial impl. I would essentially make a runtime/runtime-pure cross platform project, a runtime/runtime-native project that uses it, then conformance and protoc-gen-kotlin set of native versions. After that, maybe even runtime/runtime-pure-js and runtime-pure-jvm if Kotlin MPP doesn't support using a common-only project directly.

@phcoder
Copy link
Author

phcoder commented Jan 28, 2019

  • I think it's reasonable to have Marshaller/Unmarshaller/Sizer/Util to be interfaces with actual implementation determined at runtime (or compile-time).
  • For compatibility: The problem I have experienced is replicating the constructor of (Un)Marshaller from Input/OutputStream but making it a function instead of constructor keeps source compatibility fully without placing restrictions on class structure
  • For Util: we probably need few tests to ensure correct handling of incorrect UTF-8 and UTF-16 sequences to ensure that all implementations behave the same
  • I'm fine with you changing this PR
  • Copyright for files I contribute needs to be marked with "Copyright 2019 Google" and protobuf is "Copyright 2008 Google". Copying headers was the easiest way to achieve this but I'm fine with other approaches like e.g. refrencing license by name instead of by copy

@cretz
Copy link
Owner

cretz commented Jan 28, 2019

Thanks for the response. I have to admit I don't have the immediate time to incorporate this but hopefully I can revisit it soon.

@phcoder
Copy link
Author

phcoder commented Jan 28, 2019

If you want I can do the changes you requested and update this PR

This provides native variants for all the components.

Code is adapted from java protobuf code.
@phcoder
Copy link
Author

phcoder commented Jan 28, 2019

I added some flexibility and changed it to use JS/JVM tools on those platforms. By looking through protobufjs it looks like it implements same things as kotlin-pure code and doesn't get advantage of any js functions. So it feels like protobufjs doesn't provide any value. Should we make js and native both use kotlin pure?

@LiewJunTung
Copy link

Writing this to show my support for this feature! 🙌

@cretz
Copy link
Owner

cretz commented Mar 13, 2019

@LiewJunTung and @phcoder - Sorry I haven't done more here, I haven't been using Kotlin as much lately in my day job. In general I would take this and make a set of "pure" projects to not disturb the existing projects. Can't make any promises when I can revisit though, sorry.

garyp pushed a commit to streem/pbandk that referenced this pull request Aug 10, 2020
Originally based on cretz/pb-and-k#15. Fixes

This is a rebase of #29 onto the latest version of master.
garyp pushed a commit to streem/pbandk that referenced this pull request Aug 10, 2020
Originally based on cretz/pb-and-k#15. Fixes

This is a rebase of #29 onto the latest version of master.
garyp pushed a commit to streem/pbandk that referenced this pull request Aug 10, 2020
Originally based on cretz/pb-and-k#15. This is a
rebase of #29 onto the latest version of master.

Fixes #19.
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