Generate unbound methods

[plaflamme]

References to other Issues or PRs

Fixes #1649

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

This is a first pass at implementing the new option to generate methods for methods that have no http annotation.
When the option is enabled and a method has no options (either through annotations or external configuration), default options are generated and applied.

Other comments

This is WIP since it probably needs more tests and would need a corresponding change to gen-swagger

[johanbrandhorst]

This is a great start, it seems even simpler than I had anticipated., great work! You're right that we'll need to enable a similar thing in the swagger generator so that they can still be used together, and ideally I'd like to see a new example proto generation test that makes use of this generator flag. See the Makefile for examples of this today.

[plaflamme]

@johanbrandhorst For the example proto, should this new example not affect any of the existing ones? I'm asking because if I add generate_unbound_methods=true to the generator, it'll affect the a_bit_of_everything example (it has an unbound method). I can make a completely separate example if you think that's more appropriate.

[johanbrandhorst]

@johanbrandhorst For the example proto, should this new example not affect any of the existing ones? I'm asking because if I add generate_unbound_methods=true to the generator, it'll affect the a_bit_of_everything example (it has an unbound method). I can make a completely separate example if you think that's more appropriate.

Yes, sorry for not being clear, I think it should be a separate example. Don't enable it across the board.

[plaflamme]

@johanbrandhorst I've added the requested example in 5339757 and the resulting generated files in 498c5a4

I have to admit that the changes to the Makefile are pretty verbose. Any suggestions to simplify things are welcome, I'm not particularly knowledgeable of Makefile composability.

[johanbrandhorst]

Great work! Lets remove the swagger client generation, it doesn't add much value.

[plaflamme]

@johanbrandhorst I've added a few commits to address your comments. The only outstanding thing is whether we should run swagger-codegen on the swagger file. I think either way is fine, so please let me know what you prefer.

[johanbrandhorst]

@johanbrandhorst I've added a few commits to address your comments. The only outstanding thing is whether we should run swagger-codegen on the swagger file. I think either way is fine, so please let me know what you prefer.

You've convinced me that it is valuable to have, since it checks that our automatic annotations must produce valid swagger files, as defined by the third party generator we're using, so lets leave them in.

[johanbrandhorst]

The last thing we need to do is take a pass over the existing documentation and see if there's anything that could do with amending with this change. I can't think of anything off the top of my head, but I think it would be cool to add a section to both the intro and the more detailed pages that you can now use the gateway generator without any annotations, which hopefully opens it up to some new users. The docs are in Markdown files in the docs directory.

[plaflamme]

@johanbrandhorst I've added aaf2c61 for the documentation, let me know what you think

[johanbrandhorst]

The docs additions look great - just one more thought; I think we should add this option straight to the README too. Step 2 in the getting started guide talks about adding the annotations - we can add a step (2.a) that mentions how to use the gateway without adding any annotations at all, and move the existing step 2 to (2.b). What do you think?

Really great work on this Philippe, very happy to have you as a contributor 😄.

[plaflamme]

@johanbrandhorst Glad I can help! Please take a look, I took a stab at re-jiggering the README to put this in more prominently. Here's the rendered version: https://github.com/plaflamme/grpc-gateway/blob/generate-unbound-methods/README.md

[johanbrandhorst]

Great job, just a few comments on how we want to format the protoc invocations. I think these new invocations are a bit clearer (removes a lot of =). What do you think?

[plaflamme]

@johanbrandhorst I've added your suggestions and made another README fixup in 3249fb0 please take a look

[johanbrandhorst]

Awesome, thanks again for this Philippe, it's been a privilege to work with you!

[johanbrandhorst]

Could you please cherry pick this against v2? I expect there'll be a few merge conflicts, so let me know if you need some help.

[johanbrandhorst]

Hi Philippe, just checking, do you intend to cherry pick this against v2? I'm waiting for this before making another beta release of v2, and if you're busy I can complete the cherry picking myself, with your permission.

[plaflamme]

@johanbrandhorst I did do the cherry pick locally, but it turned out with a lot of conflicts in places that I'm not familiar with. I don't think I can invest the necessary time to learn the codebase enough to confidently resolve the conflicts... Please go ahead and do the cherry-picking, I would appreciate it.

Also, you had mentioned that it could potentially become the default behaviour in v2. Do you plan on making turning this into a different option, perhaps something like no_unbound_methods?

[johanbrandhorst]

I see, thanks, I'll take care of the cherry pick. I'll also see if we need to change the name if we change the default behavior, but I don't think so, we'll just add a note to the migration guide.