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

Replace our own custom option with the one defined by Google #12

Merged
merged 29 commits into from May 9, 2015

Conversation

Projects
None yet
5 participants
@yugui
Member

yugui commented May 4, 2015

Background

The new option brings:

  • More expressiveness of method mapping
  • Compatibility to api definitions released by Google

Overview

  • Import the definition files of the custom option from github.com/google/googleapis.
  • Deprecate our own custom option
  • Extract the parse process of CodeGeneratorRequest from code template execution
  • Add a parser of path pattern rule defined in googleapis
  • Add a stack machine which executes the parsed rule
  • Add a compiler which compiles the parsed rule into opcodes in the stack machine
  • Redesign the code template on top of this new stuff
  • Update examples
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur May 4, 2015

Contributor

Hi @yugui,

Thank you for your excellent work on this!

I've been testing out an earlier version of this branch. I'll be updating to the latest and giving it a try next.

Contributor

dmitshur commented May 4, 2015

Hi @yugui,

Thank you for your excellent work on this!

I've been testing out an earlier version of this branch. I'll be updating to the latest and giving it a try next.

Show outdated Hide outdated README.md
var _ codes.Code
var _ io.Reader
var _ = runtime.String

This comment has been minimized.

@dmitshur

dmitshur May 5, 2015

Contributor

You may want to insert var _ = json.Marshal or similar here, otherwise it will potentially generate Go code that does not compile:

....pb.gw.go:13: imported and not used: "encoding/json"

That will only happen if none of the services have a body or parameters, hence json package goes unused.

@dmitshur

dmitshur May 5, 2015

Contributor

You may want to insert var _ = json.Marshal or similar here, otherwise it will potentially generate Go code that does not compile:

....pb.gw.go:13: imported and not used: "encoding/json"

That will only happen if none of the services have a body or parameters, hence json package goes unused.

This comment has been minimized.

@yugui

yugui May 5, 2015

Member

Done.

@yugui

yugui May 5, 2015

Member

Done.

package google.api;
import "google/api/http.proto";

This comment has been minimized.

@dmitshur

dmitshur May 5, 2015

Contributor

Would you consider changing this import?

-import "google/api/http.proto";
+import "github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto";

The motivation is that, for generating .pb.gw.go files in user programs, users would typically do something like this, as described in README:

protoc -I/usr/local/include -I. -I$GOPATH/src \
 --grpc-gateway_out=logtostderr=true:. \
 path/to/your_service.proto

That would cause the google/api/http.proto import to not be found. It would require users to add an additional verbose include path directive:

 protoc -I/usr/local/include -I. -I$GOPATH/src \
+ -I$GOPATH/src/github.com/gengo/grpc-gateway/third_party/googleapis \
  --grpc-gateway_out=logtostderr=true:. \
  path/to/your_service.proto

Which is extra work that can be eliminated. What do you think?

To make it work, all imports google/api/http.proto would be need to be updated to github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto. It also seems to be more in spirit of Go to use a full import path rather than a relative one.

@dmitshur

dmitshur May 5, 2015

Contributor

Would you consider changing this import?

-import "google/api/http.proto";
+import "github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto";

The motivation is that, for generating .pb.gw.go files in user programs, users would typically do something like this, as described in README:

protoc -I/usr/local/include -I. -I$GOPATH/src \
 --grpc-gateway_out=logtostderr=true:. \
 path/to/your_service.proto

That would cause the google/api/http.proto import to not be found. It would require users to add an additional verbose include path directive:

 protoc -I/usr/local/include -I. -I$GOPATH/src \
+ -I$GOPATH/src/github.com/gengo/grpc-gateway/third_party/googleapis \
  --grpc-gateway_out=logtostderr=true:. \
  path/to/your_service.proto

Which is extra work that can be eliminated. What do you think?

To make it work, all imports google/api/http.proto would be need to be updated to github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto. It also seems to be more in spirit of Go to use a full import path rather than a relative one.

This comment has been minimized.

@slimsag

slimsag May 5, 2015

I think that changing the import path is perhaps incorrect: google/api/http.proto seems to be the canonical import path as files like these in the official Google repository import it that way.

It also seems to be more in spirit of Go to use a full import path rather than a relative one.

In Go I would certainly agree, but with protoc I don't think there is any such preference.

@slimsag

slimsag May 5, 2015

I think that changing the import path is perhaps incorrect: google/api/http.proto seems to be the canonical import path as files like these in the official Google repository import it that way.

It also seems to be more in spirit of Go to use a full import path rather than a relative one.

In Go I would certainly agree, but with protoc I don't think there is any such preference.

This comment has been minimized.

@yugui

yugui May 5, 2015

Member

I agree with @slimsag. google/api/http.proto looks to be more appropriate as a canonical path than github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto.

BTW, I should have added -I$GOPATH/src/github.com/gengo/grpc-gateway/third_party/googleapis in the example in README.md.

@yugui

yugui May 5, 2015

Member

I agree with @slimsag. google/api/http.proto looks to be more appropriate as a canonical path than github.com/gengo/grpc-gateway/third_party/googleapis/google/api/http.proto.

BTW, I should have added -I$GOPATH/src/github.com/gengo/grpc-gateway/third_party/googleapis in the example in README.md.

This comment has been minimized.

@dmitshur

dmitshur May 5, 2015

Contributor

That sounds good to me, thanks for thinking about it. google/api/http.proto indeed seems better if it's the canonical path even after vendoring.

@dmitshur

dmitshur May 5, 2015

Contributor

That sounds good to me, thanks for thinking about it. google/api/http.proto indeed seems better if it's the canonical path even after vendoring.

yugui added some commits May 5, 2015

Remove decoder configurtion from descriptor.Body
This is because all methods will share the same set of decoders.
https://groups.google.com/d/msg/grpc-io/Xqx80hG0D44/1gwmwBcnNScJ

Tentatively json.Decoder is hard-coded in the code template.
We are also going to support application/x-www-form-urlencoded and
eventually it will get pluggable by commandline flag.
Show outdated Hide outdated README.md

yugui added a commit that referenced this pull request May 9, 2015

Merge pull request #12 from gengo/feature/google-http-rule
Replace our own custom option with the one defined by Google

@yugui yugui merged commit 4747d51 into master May 9, 2015

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur May 9, 2015

Contributor

Is the implementation of query parameters complete?

Contributor

dmitshur commented May 9, 2015

Is the implementation of query parameters complete?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur May 9, 2015

Contributor

By the way, thank you very much for your excellent work on this @yugui, it is very helpful and much appreciated!

Contributor

dmitshur commented May 9, 2015

By the way, thank you very much for your excellent work on this @yugui, it is very helpful and much appreciated!

@yugui

This comment has been minimized.

Show comment
Hide comment
@yugui

yugui May 11, 2015

Member

@shurcooL Not yet. But it was enough to replace the old implementation.
I am working on query parameters and other updates from Wolfgang in another branch #14.

Member

yugui commented May 11, 2015

@shurcooL Not yet. But it was enough to replace the old implementation.
I am working on query parameters and other updates from Wolfgang in another branch #14.

@yugui yugui deleted the feature/google-http-rule branch Aug 31, 2015

ithinker1991 referenced this pull request in tronprotocol/grpc-gateway Apr 26, 2018

Merge pull request #12 from tronprotocol/feature/add_api
change amount type to int64.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment