protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

compiler/protogen: support naming a GeneratedFile import with an alias

Open noahdietz opened this issue 5 years ago • 4 comments

Is your feature request related to a problem? Please describe. In the google-cloud-go generated libraries, a single client imports a variety of modules. Some of these modules have the same package name (e.g. google.golang.org/grpc and google.golang.org/api/transport/grpc). In the code generator for these libraries, which is implemented as a protoc plugin (using the github.com/golang/protobuf v1 lib + a bunch of hand written helpers), we alias some of these conflicting imports (e.g. google.golang.org/grpc is referenced normally as grpc, while google.golang.org/api/transport/grpc is aliased to gtransport) in the generated code.

I've been looking to migrate the google-cloud-go client library generator to the google.golang.org/protobuf/compiler/protogen lib, but found the inability to alias the imports of a GeneratedFile to be a not ideal. Using the same example, we get google.golang.org/api/transport/grpc aliased as grpc2.

I'd like to be able to generate the exact same code I could using the v1 plugin lib, but without using some of the custom libs we've written that are made mostly redundant by the new protogen APIs.

Describe the solution you'd like I think it would be valuable to allow aliasing imports of a GeneratedFile to give code generator authors more flexibility where the default behavior of protogen import naming is not ideal.

Describe alternatives you've considered I will continue using the hand-spun Go import management code we've used and directly write the import block to the GeneratedFile.

If I misunderstood how to use the import APIs, please let me know! Thanks for the consideration.

noahdietz avatar Sep 18 '20 22:09 noahdietz

This is purely a cosmetic issue, and only occurs when packages are poorly named (such that two packages commonly imported at the same time have conflicting names).

On one hand, this does affect the generated package documentation. It wouldn't be difficult to support setting the local import name for a package, perhaps with a function like:

// ImportAs ensures a package is imported by the generated file.
//
// The package will be imported under the specified local package name.
// ImportAs must be called before any calls to Import, QualifiedGoIdent, or P.
func (g *GeneratedFile) ImportAs(importPath GoImportPath, packageName GoPackageName) {}

On the other hand, this is additional complexity for a case which is better resolved by choosing package names which are unique in context. (I realize that changing the package name at this time is probably not an option for you.)

neild avatar Sep 18 '20 23:09 neild

I should note that ImportAs would be best-effort, since we would presumably still prioritize correctness over cosmetics in the event that there is still a conflict over the chosen name.

dsnet avatar Sep 19 '20 00:09 dsnet

Sounds reasonable. Best-effort should be perfectly addequate for generating documentation.

puellanivis avatar Sep 21 '20 09:09 puellanivis

Thanks everyone for considering this and responding so quickly.

Yes, acknowledged, this is purely cosmetic, and best effort is sufficient. Agreed correctness should be prioritized over cosmetics in the event of a naming collision. The proposed API and documented semantics look good to me.

On one hand, this does affect the generated package documentation

Yes and in the google-cloud-go client libs, there are some poorly named packages that we've given local import names via the generator that definitely improve the package docs imo.

On the other hand, this is additional complexity for a case which is better resolved by choosing package names which are unique in context. (I realize that changing the package name at this time is probably not an option for you.)

Acknowledged, and indeed it is too late to change some of these.

noahdietz avatar Sep 21 '20 16:09 noahdietz