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

Kotlinx Serialization - Code gen Prototype #13

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marcoferrer
Copy link
Owner

@marcoferrer marcoferrer commented Dec 3, 2018

@RdeWilde
Copy link

@marcoferrer Is this safe to test yet?

Copy link

@mattdkerr mattdkerr left a 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 ?

Copy link
Owner Author

@marcoferrer marcoferrer left a 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 over class ?

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.

@marcoferrer
Copy link
Owner Author

@mattdkerr are you available on the Kotlin slack? I wanted to get some feedback on a few things.

@mattdkerr
Copy link

I am- just not very active. My alias is mkerr

Copy link

@mattdkerr mattdkerr left a 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: :shipit: complete! all files reviewed, all discussions resolved

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

Successfully merging this pull request may close these issues.

3 participants