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.

Sign up for GitHub

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

Jump to bottom

Generator for .proto files based on serializable Kotlin classes #1255

Merged
merged 6 commits into from
Apr 26, 2021
Merged

Generator for .proto files based on serializable Kotlin classes #1255

merged 6 commits into from
Apr 26, 2021

Conversation

shanshin
Copy link
Contributor

Resolves #34

@shanshin shanshin requested a review from sandwwraith December 14, 2020 09:03
@shanshin shanshin force-pushed the protobuf-generator-34 branch 2 times, most recently from 120115d to 60edb7a Compare December 14, 2020 10:27
sandwwraith
sandwwraith requested changes Dec 15, 2020
View reviewed changes
Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Publication seems to fail on CI — can you check it?

...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf-generator/jvmTest/resources/FieldNumberClass.proto Outdated Show resolved Hide resolved
formats/protobuf-generator/jvmTest/resources/AbstractHolder.proto Outdated Show resolved Hide resolved
...ts/protobuf-generator/jvmTest/src/kotlinx/serialization/protobuf/generator/GenerationTest.kt Outdated Show resolved Hide resolved
@shanshin shanshin force-pushed the protobuf-generator-34 branch 2 times, most recently from 28e0fff to a8d794a Compare December 28, 2020 14:41
sandwwraith
sandwwraith reviewed Jan 22, 2021
View reviewed changes
Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure if we should mangle incorrect serial names instead of throwing an exception

...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
...ts/protobuf-generator/jvmTest/src/kotlinx/serialization/protobuf/generator/GenerationTest.kt Outdated Show resolved Hide resolved
...ats/protobuf-generator/commonMain/src/kotlinx.serialization.protobuf.generator/Generation.kt Outdated Show resolved Hide resolved
@sandwwraith
Copy link
Member

Also, please investigate why the build is failing

@qwwdfsad qwwdfsad mentioned this pull request Feb 23, 2021
Closed
@shanshin shanshin force-pushed the protobuf-generator-34 branch 3 times, most recently from 558e896 to 2d8aafc Compare March 18, 2021 02:33
@shanshin shanshin force-pushed the protobuf-generator-34 branch from 2d8aafc to 65d6348 Compare April 19, 2021 09:13
@shanshin shanshin force-pushed the protobuf-generator-34 branch from 65d6348 to 3c6cabe Compare April 19, 2021 10:38
sandwwraith
sandwwraith requested changes Apr 19, 2021
View reviewed changes
Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! What's left to do is to tide up warning messages, finish documentation and come up with a good name

formats/protobuf/jvmTest/resources/common/schema.proto
// WARNING: This field is marked as nullable but it does not support null values
repeated int32 key = 1;
// WARNING: This field is marked as nullable but it does not support null values
repeated int32 value = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to duplicate these classes in both this one and individual files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth adding reading data from classes generated by protoc. For protoc to work, it is necessary that messages do not repeat in files - the easiest way to achieve this is by generating just one common proto file.
M.b. it should be implemented later.

formats/protobuf/jvmTest/resources/common/schema.proto Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
qwwdfsad
qwwdfsad requested changes Apr 21, 2021
View reviewed changes
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/Generation.kt Outdated Show resolved Hide resolved
@shanshin shanshin force-pushed the protobuf-generator-34 branch from c15c454 to 7fd492a Compare April 23, 2021 11:21
@shanshin shanshin force-pushed the protobuf-generator-34 branch from 7fd492a to 741e66b Compare April 23, 2021 12:57
@qwwdfsad qwwdfsad self-requested a review April 23, 2021 15:41
qwwdfsad added 2 commits April 23, 2021 19:32
@qwwdfsad
Schema generator improvements:
0cb2df8 
    * More granular documetation
    * Convinience method with single descriptor
    * Alll the implementation is encapsulated into the object body
@qwwdfsad
Update api udmp
7eb071f
@LouisCAD
Copy link
Contributor

Several typos in the commit message of the "Schema generator improvements" commit @qwwdfsad FYI.

Didn't check code changes because I'm on mobile, and the app won't let me see the "large diff".

sandwwraith
sandwwraith reviewed Apr 26, 2021
View reviewed changes
...ats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/ProtoBufSchemaGenerator.kt
* has the following list of restrictions:
*
* * Serial name of the class and all its fields should be a valid Proto [identifier](https://developers.google.com/protocol-buffers/docs/reference/proto2-spec)
* * Nullable values are allowed only for Kotlin [nullable][SerialDescriptor.isNullable] types, but not [optional][SerialDescriptor.isElementOptional]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the restriction of encoder, not schema generator, isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still worth mentioning it here unless we have our ProtoBuf doc rephrased

@qwwdfsad qwwdfsad merged commit 75566cc into dev Apr 26, 2021
@qwwdfsad qwwdfsad deleted the protobuf-generator-34 branch April 26, 2021 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandwwraith sandwwraith sandwwraith approved these changes

@qwwdfsad qwwdfsad Awaiting requested review from qwwdfsad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shanshin @sandwwraith @LouisCAD @qwwdfsad