-
Notifications
You must be signed in to change notification settings - Fork 29
KRPC-143 Repeated types in gRPC #328
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
Conversation
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.
nice, lgtm
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.
Pull Request Overview
This PR implements support for repeated types in the Protobuf plugin to extend gRPC functionality. Key changes include the addition of a new proto file (repeated.proto), updates to the gRPC service and corresponding tests, and modifications to the model and code generators to properly handle repeated fields.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
protobuf-plugin/src/test/proto/repeated.proto | Added a new message with repeated fields for testing |
protobuf-plugin/src/test/proto/reference_service.proto | Updated service to include a new RPC for repeated types |
protobuf-plugin/src/test/kotlin/kotlinx/rpc/protobuf/test/TestReferenceService.kt | Added test cases for the new repeated type functionality |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/model/FieldDeclaration.kt | Modified the repeated field type representation |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ProtoToModelInterpreter.kt | Changed the wrapping logic for field type references |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ModelToKotlinGenerator.kt | Updated getter/setter generation and type casting for repeated fields |
Comments suppressed due to low confidence (2)
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/model/FieldDeclaration.kt:18
- [nitpick] Consider renaming the 'List' data class to avoid confusion with Kotlin's standard List type. A name like 'RepeatedField' might convey its purpose more clearly.
data class List(val value: FieldType) : FieldType {
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ModelToKotlinGenerator.kt:203
- [nitpick] Ensure that appending 'List' to the field name for repeated fields aligns with the expected accessor naming conventions in the generated code, as the tests refer to properties without the 'List' suffix.
val javaName = when (field.type) { is FieldType.List -> "${field.name}List" else -> field.name }
val fq by value.value | ||
importRootDeclarationIfNeeded(fq, "toPlatform", true) | ||
} | ||
is FieldType.IntegralType -> "" |
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.
[nitpick] Double-check that numeric list elements do not require any platform-specific casting. If new numeric types are added in the future, consider handling them explicitly.
is FieldType.IntegralType -> "" | |
is FieldType.IntegralType -> ".map { it }" |
Copilot uses AI. Check for mistakes.
Subsystem
gRPC
Solution
Support for
repeated
types in the Protobuf plugin