-
Notifications
You must be signed in to change notification settings - Fork 26
KRPC-146 Nested types in gRPC #331
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
base: grpc/repeated
Are you sure you want to change the base?
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.
Pull Request Overview
This PR adds support for nested types in gRPC by updating proto definitions, code generation, and adding corresponding tests. Key changes include updating proto files (importing nested.proto and adding a new Nested rpc), modifying model and name resolution utilities to support nested names, and enhancing tests with a new testNested method to validate nested types.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
protobuf-plugin/src/test/proto/reference_service.proto | Added nested.proto import and a new rpc call for Nested messages |
protobuf-plugin/src/test/proto/nested.proto | Updated nested message definitions with optional fields to support nesting |
protobuf-plugin/src/test/kotlin/kotlinx/rpc/protobuf/test/TestReferenceService.kt | Added a new test method (testNested) to verify nested type behavior |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/model/NameResolver.kt | Added a pprint method for debugging hierarchical names |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/model/MessageDeclaration.kt | Changed field collections from Sequence to List for deterministic ordering |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/model/FqName.kt | Updated fullName to support an optional classSuffix and added fullNestedName |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ProtoToModelInterpreter.kt | Replaced sequence operations with list mappings for better consistency |
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ModelToKotlinGenerator.kt | Adjusted naming resolution and generation logic to properly handle nested types |
Files not reviewed (1)
- protobuf-plugin/build.gradle.kts: Language not supported
Comments suppressed due to low confidence (2)
protobuf-plugin/src/test/proto/nested.proto:19
- [nitpick] Using 'string' as a field name might be confusing given it matches the type name. Consider renaming it to a more descriptive identifier, for example 'messageString' or another contextual name.
string string = 3;
protobuf-plugin/src/main/kotlin/kotlinx/rpc/protobuf/ModelToKotlinGenerator.kt:196
- [nitpick] The updated use of fullNestedName to build the platformType ensures nested types are fully qualified; please verify that this naming change does not unintentionally affect any downstream references or generated API contracts.
val platformType = "${declaration.outerClassName.safeFullName()}.${declaration.name.fullNestedName()}"
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.
lgtm
// KRPC-147 OneOf Types | ||
// declaration.oneOfDeclarations.forEach { oneOf -> | ||
// generateOneOf(oneOf) | ||
// } | ||
// | ||
// KRPC-146 Nested Types |
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
Subsystem
gRPC
Problem Description
Proto files support nesting of declarations
Solution
Provide support in code gen