Add module generator option #1400
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #1400 +/- ##
==========================================
+ Coverage 54.14% 54.22% +0.07%
==========================================
Files 42 42
Lines 4375 4389 +14
==========================================
+ Hits 2369 2380 +11
- Misses 1750 1752 +2
- Partials 256 257 +1
Continue to review full report at Codecov.
|
|
Hi Humphrey, thanks for your interest in the project! Let me understand how this overlaps with |
|
Hey Johan, thanks for looking at this so quickly! Recently I've been trying to upgrade our protobuf dependencies to later versions and I hit some snags with the output directories due to our current directory structure (which needs changing anyway). The following is an example which uses the Currently we have a folder which contains quite a few of our apis called ...which included The issue I then found was that the This change allows me to run: which respects the We've been moving towards a better directory structure for the apis so that they're not all under |
| if g.modulePath != "" { | ||
| if g.pathType != pathTypeImport { | ||
| return "", errors.New("cannot use module= with paths=source_relative") | ||
| } | ||
| trimPath, pkgPath := g.modulePath+"/", file.GoPkg.Path+"/" | ||
| if !strings.HasPrefix(pkgPath, trimPath) { | ||
| return "", fmt.Errorf("%v: file go path does not match module prefix: %v", file.GoPkg.Path, trimPath) | ||
| } | ||
| name = filepath.Join(strings.TrimPrefix(pkgPath, trimPath), filepath.Base(name)) | ||
| } else if g.pathType == pathTypeImport && file.GoPkg.Path != "" { | ||
| // Only respect the path value if module is not supplied | ||
| name = fmt.Sprintf("%s/%s", file.GoPkg.Path, filepath.Base(name)) | ||
| } |
There was a problem hiding this comment.
Whenever I see something like else if I think it can usually be done neater with a switch statement. What do you think about:
| if g.modulePath != "" { | |
| if g.pathType != pathTypeImport { | |
| return "", errors.New("cannot use module= with paths=source_relative") | |
| } | |
| trimPath, pkgPath := g.modulePath+"/", file.GoPkg.Path+"/" | |
| if !strings.HasPrefix(pkgPath, trimPath) { | |
| return "", fmt.Errorf("%v: file go path does not match module prefix: %v", file.GoPkg.Path, trimPath) | |
| } | |
| name = filepath.Join(strings.TrimPrefix(pkgPath, trimPath), filepath.Base(name)) | |
| } else if g.pathType == pathTypeImport && file.GoPkg.Path != "" { | |
| // Only respect the path value if module is not supplied | |
| name = fmt.Sprintf("%s/%s", file.GoPkg.Path, filepath.Base(name)) | |
| } | |
| switch { | |
| case g.modulePath != "" && g.pathType != pathTypeImport: | |
| return "", errors.New("cannot use module= with paths=source_relative") | |
| case g.modulePath != "": | |
| trimPath, pkgPath := g.modulePath+"/", file.GoPkg.Path+"/" | |
| if !strings.HasPrefix(pkgPath, trimPath) { | |
| return "", fmt.Errorf("%v: file go path does not match module prefix: %v", file.GoPkg.Path, trimPath) | |
| } | |
| name = filepath.Join(strings.TrimPrefix(pkgPath, trimPath), filepath.Base(name)) | |
| case g.pathType == pathTypeImport && file.GoPkg.Path != "": | |
| // Only respect the path value if module is not supplied | |
| name = fmt.Sprintf("%s/%s", file.GoPkg.Path, filepath.Base(name)) | |
| } |
|
Thanks for your contribution! Could you please cherry pick this against the v2 branch? |
* Add module generator option * Feedback from code review
|
Awesome, thanks @johanbrandhorst! I've made a cherry pick on the v2 branch here: #1409 |
The go protobuf compiler recently added a
moduleoption define a module prefix which is trimmed from the name of go package to determine the output directory. Currently when usingpaths=importa protobuf file with thego_packagebeingexample.com/path/to/examplewould be output under./example.com/path/to/example/myfile.pb.go. This would usually be fine withGOROOTbut with go modules it's less useful as the output directory structure does not have to beexample.com/path/toetc.I have tried to mirror the behaviour from the protobuf generator. The commit for which is here: https://go-review.googlesource.com/c/protobuf/+/219298/