-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for protobuf editions #54
base: main
Are you sure you want to change the base?
Add support for protobuf editions #54
Conversation
cmd/protoc-gen-go-grpcmock/main.go
Outdated
gen.SupportedFeatures = uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL) | uint64(pluginpb.CodeGeneratorResponse_FEATURE_SUPPORTS_EDITIONS) | ||
gen.SupportedEditionsMinimum = descriptorpb.Edition_EDITION_PROTO2 | ||
gen.SupportedEditionsMaximum = descriptorpb.Edition_EDITION_2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solidDoWant Thank you for your contribution! Adding support for editions is really appreciated. The only thing I'm a bit skeptical about is specifying a min/max version.
What is the benefit of adding the minimum and maximum supported versions here? To my knowledge we are not making use of any version specific features. Adding a maximum version means that we would have to maintain this from now on and bump it every time a new edition comes out, which I would like to avoid if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a maximum version means that we would have to maintain this from now on and bump it every time a new edition comes out, which I would like to avoid if possible.
Completely get that. Unfortunately I think it's required. I did some local testing and without these set, the generator with a very generic error about unsupported versions. Every other tool with editions support seems to have these set.
I think I found a workaround to solve the issue. protoc-gen-go
sets these using constants, which the upstream project updates when new versions release. 58237fb sets the values to these variables, so while they will need to be updated upstream (and a new release cut), the code itself won't need to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's unfortunate. Though in this scenario, I'd prefer to explicitly define the constants for Min/MaxSupportedVersions (sorry for the back and forth) since I think it would be weird to tie this to a specific protoc-gen-go
version. If we have to increment these every time a new edition is released, then I'd prefer it to actually be a code change instead of changing something in the background by just updating the dependencies.
Would you be able to also add a file with editions syntax to the examples? E.g. just taking https://github.com/lovoo/protoc-gen-go-grpcmock/blob/main/examples/helloworld/helloworld.proto and add another version of this file with editions syntax? Just so we can verify that we are able to support it?
…ions Signed-off-by: solidDoWant <fred.heinecke@yahoo.com>
Version bump + 4 line change to add support for protobuf editions up to 2023 (current latest supported by protoc-gen-go). This has no effect on the program output, but without it,
protoc
will fail withon any proto files that use
edition
instead ofsyntax
.