option to tweak generated Register* function names

[jhump]

We have our own protoc plugin that wants to generate a Register<ServiceName>Handler function: https://godoc.org/github.com/fullstorydev/grpchan

If I use both the grpchan and grpc-gateway protoc plugins, they generate code that won't compile. I would really prefer the grpc-gateway functions be named Register<ServiceName>HTTPHandler to make the distinction more obvious from the name.

So this pull request provides an option that can be used to change the generated name. The option is register-func-suffix and it defaults "Handler" to preserve the names if the option is not used.

I can then do this in my own go:generate directive to get the desired output:

protoc -I $HOME/go/src \
    -I $HOME/go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
    --go_out=plugins=grpc:$HOME/go/src \
    --grpchan_out=$HOME/go/src \
    --grpc-gateway_out=logtostderr=true,allow_delete_body=true,register_func_suffix=HTTPHandler:$HOME/go/src \
    $HOME/go/src/github.com/myorg/myrepo/mypackage/myprotocol.proto

[jhump]

@achew22, are you the right person to ping if I'm hoping to get a change reviewed?

[jhump]

Ping @achew22?

[achew22]

Looking at this, I don't think this is a change that I would be comfortable making. This would be a breaking change for every single user of the project in search of compatibility that I don't think is widely needed. If this becomes a community standard and part of every major gRPC installation (or gets rolled into go-grpc-core) then I would definitely be receptive to making this change.

If I may ask a higher level question. Given that there is a collision on that name right now, it seems likely there will be more collisions. Do you intend to run down every other project who is potentially going to conflict on that generated name? Maybe it would be a good idea to switch your handler to Register<ServiceName>ChannelHandler?

If this is a change that you think absolutely must be implemented, I think that making this a configurable flag in the generator followed by a slow migration and a major version upgrade hard cutover is the way to go. I will warn you that the bar for testing it at an acceptable level will be really high and will involve probably duplicating all of this projects integration tests to ensure that we don't break anyone in either configuration.

Sorry to not give a happy, "let's merge this", but I have concerns WRT major breaking changes like this.

[jhump]

This would be a breaking change for every single user of the project

How so? It behaves exactly as it does today unless requested via plugin arg.

I think that making this a configurable flag in the generator

That's what I've done.

Sorry to not give a happy, "let's merge this", but I have concerns WRT major breaking changes like this.

I would not expect you to accept a breaking change. Please take a longer look at the PR description and code.

[jhump]

PIng @achew22. If you don't mind reviewing. This is not a breaking change. Please take a closer look at the PR description and the code.

[achew22]

I just got a bit of time to actually look at the code (sorry for firing back so fast, things have been really crazy of late).

For the most part I'm okay with this. I would like to see something added in the examples folder that has the flag changed so we can ensure that the handler is properly renamed in all the cases it needs to be by actually compiling it. You should be able to do that in the Makefile in the root. Add a new directory with a name like examples/user_selected_handler_name (or find a better name, this is just a placeholder) and have it output to there. No need to do all the examples, just pick one ABE or something.

[jhump]

@achew22, I rebased this. I'm trying to figure out how to actually do what you requested, regarding a test that the option is working. Am I correct to be staring at Bazel build files? I don't see any existing examples/tests that exercise the other options. Maybe I am looking in the wrong place. A little more direction appreciated.

[jhump]

The build breakage was bad timing - I pushed just after protobuf merged their dev branch to master. I will rebase this PR after #636 lands, to see if I can get the tests to go green.

[codecov-io]

Codecov Report

Merging #571 into master will increase coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   58.88%   58.88%   +<.01%     
==========================================
  Files          30       30              
  Lines        2853     2863      +10     
==========================================
+ Hits         1680     1686       +6     
- Misses       1010     1014       +4     
  Partials      163      163

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/gengateway/template.go	55.76% <100%> (+0.86%)	⬆️
protoc-gen-grpc-gateway/gengateway/generator.go	43.42% <50%> (+1.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b051d...d54e528. Read the comment docs.

[jhump]

ping @achew22