-
Notifications
You must be signed in to change notification settings - Fork 29
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
Kotlinx Serialization - Code gen Prototype #13
base: develop
Are you sure you want to change the base?
Conversation
@marcoferrer Is this safe to test yet? |
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.
Reviewed 12 of 13 files at r1, 4 of 4 files at r2.
Reviewable status: 16 of 17 files reviewed, 4 unresolved discussions (waiting on @marcoferrer)
example-project/build.gradle, line 22 at r1 (raw file):
id 'idea' id 'com.google.protobuf' version '0.8.6' id 'kotlinx-serialization' version '1.3.0'
I'm guessing this needs to be updated by now
example-project/build.gradle, line 61 at r1 (raw file):
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8" implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:${versions.coroutines}" implementation "org.jetbrains.kotlinx:kotlinx-serialization-runtime:0.9.0"
same here?
protoc-gen-kroto-plus/src/main/generated/com/github/marcoferrer/krotoplus/config/MPProtobufMessagesGenOptions.java, line 1 at r1 (raw file):
// Generated by the protocol buffer compiler. DO NOT EDIT!
was this directory tree supposed to be committed?
protoc-gen-kroto-plus/src/main/kotlin/com/github/marcoferrer/krotoplus/generators/MPProtobufMessageGenerator.kt, line 9 at r1 (raw file):
object MPProtobufMessageGenerator : Generator {
is there an advantage to using object
over class
?
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.
Reviewable status: 16 of 17 files reviewed, 4 unresolved discussions (waiting on @marcoferrer and @mattdkerr)
example-project/build.gradle, line 22 at r1 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
I'm guessing this needs to be updated by now
This PR has gone stale. It was shelved until proper support of proto3 features was made available in Kotlinx.serialization. I was considering releasing a version with proto2 support in the mean time, but didnt get much interest from the community
protoc-gen-kroto-plus/src/main/generated/com/github/marcoferrer/krotoplus/config/MPProtobufMessagesGenOptions.java, line 1 at r1 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
was this directory tree supposed to be committed?
Only the generated output for the plugin configuration is committed. I was pretty torn on this decision. The reasoning behind it was that since the configuration protos are slow changing, committing the output would make two things very clear. When the proto definitions have changed and what changes in the generated code are introduced when bumping the protobuf version. Its done in grpc-java/test-proto and grpc-java/services. The key difference is that they're committing the source that their plugin outputs. One goal was to do the same in this repo once full Kotlin model support was completed.
protoc-gen-kroto-plus/src/main/kotlin/com/github/marcoferrer/krotoplus/generators/MPProtobufMessageGenerator.kt, line 9 at r1 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
is there an advantage to using
object
overclass
?
0 advantage. It was done for simplicity sake since the life cycle of the plugin is short lived. Recently I've been refactoring the internals of the plugin to make it friendlier to community contributions. The requested features started growing faster than expected. So more work has been put into building out those features to improve the user experience.
@mattdkerr are you available on the Kotlin slack? I wanted to get some feedback on a few things. |
I am- just not very active. My alias is mkerr |
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.
Reviewed 1 of 13 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved
Details to come
https://github.com/Kotlin/kotlinx.serialization
Kotlin/kotlinx.serialization#34
This change is