Skip to content
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

Use importPath to set package name rather than package path. #537

Merged
merged 3 commits into from
Feb 23, 2018

Conversation

rwlincoln
Copy link
Contributor

In #536 @shynie correctly observes that the import_path flag was not implemented correctly in #507. Although it is called import_path, in golang/protobuf the flag is used to set the package identity name.

import_path=foo/bar - used as the package if no input files declare go_package. If it contains slashes, everything up to the rightmost slash is ignored.

Currently it is used to set the package path:

This PR moves the use of the importPath field to the packageIdentityName function.

Note that eventually the "Want to support" section of the README should be updated:

https://github.com/grpc-ecosystem/grpc-gateway/blob/6658b3a4fd7017117532b54b5c6bf2fd2207ffc2/README.md#want-to-support

@achew22
Copy link
Collaborator

achew22 commented Feb 16, 2018

Does this PR mean that importPath should be working as expected? Could you add a test by modifying the makefile to invoke protoc with this parameter set to a value you know to work and output to a different directory so we could have a more e2e test?

@rwlincoln
Copy link
Contributor Author

This PR makes import_path work in the same way that it does in golang/protobuf.

Could you add a test by modifying the makefile to invoke protoc with this parameter set to a value you know to work and output to a different directory so we could have a more e2e test?

Unfortunately, all of the example .proto files include the go_package option. import_path is only used "as the package if no input files declare go_package". I will try to add some tests to descriptor/registry_test.go.

@rwlincoln
Copy link
Contributor Author

I have added tests for the SetImportPath method on the registry type. They test:

  • that importPath is only used as the package if no input files declare go_package and
  • if importPath contains slashes, everything up to the rightmost slash is ignored.

This is in accordance with the documentation:

// SetImportPath registers the importPath which is used as the package if no

@achew22
Copy link
Collaborator

achew22 commented Feb 23, 2018

Looking through this and some other documentation on import paths, I think this is the right way to do it but I wouldn't be surprised if someone came along and said "this totally breaks my use case" when we merge. If something goes when this is merged, do you mind if I @mention you in the issue and see if we can find a good resolution for everyone?

@achew22 achew22 merged commit 5c79dbf into grpc-ecosystem:master Feb 23, 2018
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…osystem#537)

* Prioritise go_package option over import_path argument.
* Adding registry tests for SetImportPath method.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants