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
Add basic Kotlin support #5409
Add basic Kotlin support #5409
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
f661b53
to
87a253c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks pretty good to me.
Big thing missing is docs. There should be a KotlinUsage.md
and a case for Kotlin in the tutorial.
7b4bd5b
to
921e01c
Compare
Identation is important for some languages and different projects have different ways of ident code, e.g. tabs vs spaces, so we are adding optional support on CodeWriter for identation.
This change adds support for generating Kotlin classes. The approach of this generator is to keep it as close as possible to the java generator for now, in order to keep the change simple. It uses the already implemented java runtime, so we don't support cross-platform nor js Kotlin yet. Kotlin tests are just a copy of the java tests.
kotlin test to TestAll.sh
Ok, LGTM now. @LouisCAD any more changes you'd like to see? Is this ready to merge? |
BTW - in the meantime, https://github.com/Kotlin/kotlinx.serialization came out. Would flatbuffers need to integrate with kotlinx.serialization to be taken seriously? (non-rhetorical question, maybe it works fine outside that framework?) |
Most serialization "frameworks" don't work with FlatBuffers, because they assume there is a de-serialization step where serialized data gets transformed into language objects. FlatBuffers doesn't work that way. This Kotlin framework seems to have the same issue, on first glance. |
Makes sense! In my particular case I'm doing the mental math of "If I go with kotlinx.serialization, I can switch to Cbor format with literally one line of code. If I go with flatbuffers, I'm making more of a commitment - how much harder will it be, and is it worth it to try to switch over" |
Well it already supports ProtoBuf, so it probably won't hurt to take a closer look... |
From the previous comments the only thing he is missing it to make the generator generates Kotlin native from start. But since I am using the java runtime this is not possible. For me, having a But since this is an implementation detail and does not affect API, later updates we can let user choose which runtime he wants (JVM or Native) and the only thing we need to be sure is that |
My understanding is that On the other hand, you could generate the "adapters/serializer" classes for |
Ok, since I have seen no more comments in a month now, I will merge! Let's discuss improvements afterwards. @paulovap thanks for your hard work! This is a nice language to support. Agreed that efficiency should come first :) FlexBuffers has the same API issue that ideally it shouldn't need to be de-serialized into language objects, but I agree that if you want to work with a format that doesn't need a code generation step, then this would be quite nice to work with. |
Ok, since I have seen no more comments in a month now, I will merge!
Let's discuss improvements afterwards.
I'm sorry I didn't complete the review in time.
I saw the mention to me made 2 weeks ago and started to review, but it was
a significant amount of files and I am in vacation.
The few files I reviewed till now looked good to me, but it's only a
fraction and I didn't try it on a machine and in a sample project.
I think it'd be great to support Kotlin/JS and Kotlin/Native indeed, and
allow generating a multiplatform API using these, but maybe something like
a pure Kotlin and JVM independent ByteBuffer analogue would be helpful
first and would remove the need to generate platform specific code
generation.
It'd also make adding support for future Kotlin supported platforms easier
(like possible future Kotlin/WASM not based on Kotlin/Native).
Are there any rules for third party libraries inclusion in case something
like that is made from someone in the community?
…On Tue, Jul 23, 2019, 1:05 AM Wouter van Oortmerssen < ***@***.***> wrote:
Merged #5409 <#5409> into
master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5409?email_source=notifications&email_token=ABVG6BIL2X3I4SH5LQ6N3S3QAY4NDA5CNFSM4HYQ7VE2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSULYZMQ#event-2501348530>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABVG6BNN7E3TLBS6MMNKC23QAY4NDANCNFSM4HYQ7VEQ>
.
|
I think we would have less maintenance burden if we breakdown Adding dependency we would need to add a build tool (gradle) setup on CI, keep updating dependency versions and etc. As interface, other projects might just plugin if they come up with some more fancy strategy. |
@LouisCAD we typically try to make this FlatBuffers repo as stand-alone as possible, so it be nice to support other platforms directly as part of the code that was just merged. |
Add Kotlin generator
This change adds support for generating Kotlin classes.
The approach of this generator is to keep it as close
as possible to the java generator for now, in order
to keep the change simple.
It uses the already implemented java runtime,
so we don't support cross-platform nor JS Kotlin yet.
Kotlin tests are just a copy of the java tests and grpc
is not supported yet.