-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
protoc-gen-go-grpc: add requirement of embedding UnimplementedServer in services #3657
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.
Reviewed 14 of 30 files at r1, 17 of 17 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dfawley)
cmd/protoc-gen-go-grpc/grpc.go, line 142 at r2 (raw file):
} if *requireUnimplemented { g.P("unimplemented", serverType, "()")
Is there a better name so that the compilation error also serves as documentation?
requireUnimplementedXXX()
?
needsToEmbedUnimplementedXXX()
?
Good idea. I went with |
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.
Reviewed 17 of 17 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dfawley)
… would be an API change
type LoadBalancerServer interface { | ||
// Bidirectional rpc to get a list of servers. | ||
BalanceLoad(LoadBalancer_BalanceLoadServer) error | ||
mustEmbedUnimplementedLoadBalancerServer() |
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.
Oh, we said we want to revert the changes to the generated code, because they are API changes.
files for gRPC. For usage information, please see our [quick start | ||
guide](https://grpc.io/docs/languages/go/quickstart/). | ||
|
||
By default, to register services using the methods generated by this tool, the |
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.
Move this to a separate section?
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.
Done.
|
||
This tool generates Go language bindings of `service`s in protobuf definition | ||
files for gRPC. For usage information, please see our [quick start | ||
guide](https://grpc.io/docs/languages/go/quickstart/). |
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.
Quick start doesn't have examples for using this plugin (yet?)
Even if it does, it would be helpful to have a copy-paste ready command here?
Or point to https://github.com/grpc/grpc-go/blob/master/regenerate.sh#L60
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.
Documentation is very much a work in progress. Let's make incremental progress on it until we eventually release the tool at 1.0.
No description provided.