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 #332

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Conversation

mterwill
Copy link
Contributor

Protobuf recently added support for field presence. In a proto file, they are denoted by the optional keyword, e.g.

message Foo {
  optional string bar = 1;
}

Support for optional fields is opt-in on a per-code-generator basis. Without adding a particular flag, you get an error:

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

As far as I can tell, there should not be anything special we need to do to support optional fields in Twirp as we only reference message types and don't do anything with their fields.

Support was previously attempted in #275 but that was closed.

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

Protobuf recently added support for [optional fields][].
These are opt-in on a per-code-generator basis.
Without this flag, you get an error:

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

As far as I can tell, there should not be anything special we need to do
to support optional fields in Twirp as we only reference message types
and don't do anything with their fields.

[optional fields]: https://github.com/protocolbuffers/protobuf/blob/v3.12.0/docs/field_presence.md#application-note-field-presence
@Solution
Copy link

Solution commented Aug 30, 2021

Can' wait to have it in official release generator.

@zulakm
Copy link
Contributor

zulakm commented Aug 30, 2021

Looks good to me, thanks for the PR!

@zulakm zulakm merged commit 93129a0 into twitchtv:main Aug 30, 2021
@mterwill mterwill deleted the optional-fields branch August 30, 2021 22:55
@Solution
Copy link

thanks a lot for merge @zulakm, it's also possible to release it as minor version?

@sitavanc
Copy link

Thank you @zulakm! It would be great if this change appeared in next release ;)

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.

4 participants