-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fixes 1398: deprecate Regiter_XXX_HandlerServer #1399
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #1399 +/- ##
=======================================
Coverage 54.14% 54.14%
=======================================
Files 42 42
Lines 4375 4375
=======================================
Hits 2369 2369
Misses 1750 1750
Partials 256 256
Continue to review full report at Codecov.
|
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.
Thanks for opening the PR! Just a small note on the new additions.
@@ -564,6 +564,7 @@ func local_request_{{.Method.Service.GetName}}_{{.Method.GetName}}_{{.Index}}(ct | |||
// Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}Server registers the http handlers for service {{$svc.GetName}} to "mux". | |||
// UnaryRPC :call {{$svc.GetName}}Server directly. | |||
// StreamingRPC :currently unsupported pending https://github.com/grpc/grpc-go/issues/906. | |||
// Deprecated :consider using Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}FromEndpoint instead. This will not provide grpc.ServerTransportStream https://github.com/grpc-ecosystem/grpc-gateway/issues/1398. This implementation will be missing certain features to behave like a standard stanalone grpc.Server. |
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 is a little heavy handed, I don't think we need to use the word "deprecated", this feature was only added earlier this year, and it does serve some users needs.
// Deprecated :consider using Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}FromEndpoint instead. This will not provide grpc.ServerTransportStream https://github.com/grpc-ecosystem/grpc-gateway/issues/1398. This implementation will be missing certain features to behave like a standard stanalone grpc.Server. | |
// Note that using this registration option will cause many gRPC library features (such as grpc.SendHeader, etc) to stop working . Consider using Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}FromEndpoint instead. |
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.
I wonder if there is a better way of warning people. In Goland, for example, It is really hard to notice the warnings, until you hover your cursor over the method for over a second or so and scroll all the way down to the note 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.
I don't think we need anything more than this. It's part of the documentation of the function, it should be one of the first places the user looks when using it.
Thanks for your contribution! Could you cherry pick this against the v2 branch please? |
* cherry-pick #1399 * remove extra * add missing test result change
Gives deprecated notice when try to use Regiter_XXX_HandlerServer. Encourage using Register_XXX_FromEndpoint for full feature set and simplicity.