-
Notifications
You must be signed in to change notification settings - Fork 435
[CodeGen Protobuf support] protoc-gen-grpc-swift v2 #1778
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
Motivation: We want to have a protoc plugin for grpc-swift v2 which builds on top of the code generator pipeline. Modifications: - Created the ProtobufCodeGenerator struct that encapsulates all setps needed to generate code for a given file descriptor - Created tests for the ProtobufCodeGenerator - added a new option for selecting v2 for the plugin - modified main() accordingly Result: The protoc plugin for grpc-swift will support v2 and will use the CodeGen library.
| /// Whether or not server code should be generated. | ||
| public var server: Bool | ||
|
|
||
| public init(accessLevel: AccessLevel, indentation: Int = 4, client: Bool, server: Bool) { |
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.
Defaulted parameters should be last (but before any trailing closures, if they exist)
| import SwiftProtobufPluginLibrary | ||
|
|
||
| public struct ProtobufCodeGenerator { | ||
| internal var configs: SourceGenerator.Configuration |
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.
There's only one configuration so the abbreviation would be 'config', however, we should just spell out configuration in full.
| internal var configs: SourceGenerator.Configuration | ||
| internal var file: FileDescriptor | ||
|
|
||
| public init(configs: SourceGenerator.Configuration, file: FileDescriptor) { |
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.
It's a bit weird to store the file descriptor because it means the generator can only ever generate code for that descriptor. It makes more sense to have it as a parameter to generateCode
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.
Yes, I followed the existing Generator model, but I will make it a parameter of the function
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.
Don't make the mistake of thinking the, or any for that matter, existing code is good 😉
| func convertOptionsToSourceGeneratortConfiguration( | ||
| _ options: GeneratorOptions | ||
| ) -> SourceGenerator.Configuration { | ||
| let accessLevel: SourceGenerator.Configuration.AccessLevel | ||
| switch options.visibility { | ||
| case .internal: | ||
| accessLevel = .internal | ||
| case .package: | ||
| accessLevel = .package | ||
| case .public: | ||
| accessLevel = .public | ||
| } | ||
| return SourceGenerator.Configuration( | ||
| accessLevel: accessLevel, | ||
| client: options.generateClient, | ||
| server: options.generateServer | ||
| ) | ||
| } |
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.
This seems more appropriate as an extension on SourceGenerator.Configuration:
extension SourceGenerator.Configuration {
init(options: GeneratorOptions) {
// ...
}
}| throw GenerationError.invalidParameterValue(name: pair.key, value: pair.value) | ||
| } | ||
|
|
||
| case "V2": |
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.
Given we're not really supporting this yet, let's prefix it with an underscore: _V2
| /// Sends a greeting. | ||
| internal func sayHello<R>( |
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.
There shouldn't be a blank line between these.
| /// The greeting service definition. | ||
| public protocol Helloworld_GreeterStreamingServiceProtocol: GRPCCore.RegistrableRPCService { |
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.
There shouldn't be a blank line between these.
| /// Sends a greeting. | ||
| func sayHello(request: ServerRequest.Stream<Helloworld.Greeter.Methods.SayHello.Input>) async throws -> ServerResponse.Stream<Helloworld.Greeter.Methods.SayHello.Output> |
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.
There shouldn't be a blank line between these.
| /// The greeting service definition. | ||
| public protocol Helloworld_GreeterServiceProtocol: Helloworld.Greeter.StreamingServiceProtocol { |
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.
There shouldn't be a blank line between these.
| /// Sends a greeting. | ||
| func sayHello(request: ServerRequest.Single<Helloworld.Greeter.Methods.SayHello.Input>) async throws -> ServerResponse.Single<Helloworld.Greeter.Methods.SayHello.Output> |
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.
There shouldn't be a blank line between these.
ceef64e to
775637b
Compare
glbrntt
left a comment
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!
Motivation: We want to have a protoc plugin for grpc-swift v2 which builds on top of the code generator pipeline. Modifications: - Created the ProtobufCodeGenerator struct that encapsulates all setps needed to generate code for a given file descriptor - Created tests for the ProtobufCodeGenerator - added a new option for selecting v2 for the plugin - modified main() accordingly Result: The protoc plugin for grpc-swift will support v2 and will use the CodeGen library.
Motivation:
We want to have a protoc plugin for grpc-swift v2 which builds on top of the code generator pipeline.
Modifications:
Result:
The protoc plugin for grpc-swift will support v2 and will use the CodeGen library.