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

Support proto3 field presence #38

Merged
merged 1 commit into from
Apr 24, 2022

Conversation

sarahegler
Copy link
Contributor

@sarahegler sarahegler commented Apr 13, 2022

Since protobuf 3.14, the optional keyword can be used to distinguish between an unset and default primitive value. However, this code generator does not currently support the optional keyword; code generation of files containing the optional keyword result in the following error:

example.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-twirpy hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional.

This update is to support code generation for protobuf schemas containing the optional keyword. It mirrors this change in the corresponding go code generator here: twitchtv/twirp#332

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ofpiyush
Copy link
Contributor

Thanks for the PR!

Can you remove the .idea directory from the PR? I'll test this out over this weekend.

@sarahegler
Copy link
Contributor Author

Thanks for looking @ofpiyush - did you have a chance to test this out?

@ofpiyush
Copy link
Contributor

@sarahegler sorry no. My dev machine conked on Saturday, I set up a new one but forgot about it by the time I'd set new one up. 🙈

I'll check today.

@ofpiyush
Copy link
Contributor

@sarahegler Thanks a ton for making this change! 🎉

@avinassh I've tested it on local, we can merge it.

I'll add an optional field to our example as well as a separate PR.

@avinassh avinassh merged commit 53a32c4 into verloop:main Apr 24, 2022
@avinassh
Copy link
Contributor

Thank you @sarahegler for the PR and thank you @ofpiyush for testing it. I have merged it now 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants