Add same_package option to protoc-gen-connect-go #803
Conversation
Signed-off-by: bufdev <[email protected]>
There was a problem hiding this comment.
I'd be open to other naming patterns for this option, including making it "enum style".
I'm not sure an enum makes sense. But maybe a "suffix" string that defaults to "connect" -- then allows users to customize the location (like in case a "connect" sub-package were somehow a conflict) and then setting it to empty string would put it in the same package.
But, TBH, I think the simple bool is actually fine.
cmd/protoc-gen-connect-go/main.go
Outdated
| } | ||
| generatedFile := plugin.NewGeneratedFile( | ||
| file.GeneratedFilenamePrefix+generatedFilenameExtension, | ||
| goImportPath, | ||
| ) | ||
| generatedFile.Import(file.GoImportPath) |
There was a problem hiding this comment.
Don't we only want to do this when !samePackage?
I think we need to come up with a testing strategy. I need to look at the repo a little more to see how existing tests work. Hopefully there's an easy way to plugin a test for this new option.
There was a problem hiding this comment.
Put up a testing strategy that runs the protoc-gen-connect-go binary by re-compiling the main.go file. Wanted to bounce this off before extending. Currently it will assert that the expected files in the output but not the contents. We could add a testdata/ directory of .pb.go files to check contents match if we wanted better testing of contents.
There was a problem hiding this comment.
We definitely need to test contents, though I don't think "golden files" are sufficient since they are verified by human eyes. We actually need to compile them to make sure we are generating valid code: this option changes references, packages, and imports, which is a high risk of a compiler error if not done correctly.
To get as much coverage as possible with this option, we may even want to generate all test files that we're currently using to test protoc-gen-connect-go, to a different package with this option and make sure it all compiles.
There was a problem hiding this comment.
Updated the testdata directory to a set of module proto files which is generated as part of make generate. I found having the golden files output in testdata nice to be able to browse the output. The test code imports these, which asserts that it is compilable and valid code, and then uses it for comparisons when running the different options.
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
same-package-option
branch
from
1038f68 to
7b04e44
Compare
Signed-off-by: Edward McFarlane <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
This is the alternative option `package_suffix` re: #803 (review). I'm not sure which one I like more - I don't like the name `same_package` that much, and this gives more flexibility, but I don't know if we want to offer that flexibility. Looking for opinions. Regardless of if we choose this or #803 as-is, we should make sure the README.md documents the option. Signed-off-by: bufdev <[email protected]>
This is the alternative option `package_suffix` re: #803 (review). I'm not sure which one I like more - I don't like the name `same_package` that much, and this gives more flexibility, but I don't know if we want to offer that flexibility. Looking for opinions. Regardless of if we choose this or #803 as-is, we should make sure the README.md documents the option. Signed-off-by: bufdev <[email protected]> Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
same-package-option
branch
from
3dbb98f to
189737f
Compare
Signed-off-by: Edward McFarlane <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
same-package-option
branch
from
0013a35 to
525b3c3
Compare
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
same-package-option
branch
from
525b3c3 to
c8a29a0
Compare
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
same-package-option
branch
from
4dd5839 to
7349179
Compare
This adds an option
same_packagetoprotoc-gen-connect-go. If specified, the connect-go types will be generated to the same package as the base types.Per #310 (reply in thread), I'm not 100% confident in this design - I'd be open to other naming patterns for this option, including making it "enum style".