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 (optional fields) #66

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

mterwill
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

What's in this PR?
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;
}

Why?
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.

Companion PR: twitchtv/twirp#332

Example Usage
See above.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

No changes are required beyond setting this flag as twirphp doesn't
interact with message types at all.

Fixes:

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

Thanks!

@sagikazarmark sagikazarmark merged commit 610c846 into twirphp:master Aug 26, 2021
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.

2 participants