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

Add support for proto3 optional presence #275

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

awbraunstein
Copy link
Contributor

@awbraunstein awbraunstein commented Apr 17, 2021

Changes

Add implementation for proto3 optional fields with presence tracking. Spec: https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md

Fixes: #263

Additionally, this change:

  • Improves tests for the proto2 scalar messages
  • Explicitly calls .buffer on the serialized message before passing into Buffer.from to handle a weird typescript issue.

Verification

Tested via unit tests and visual inspection of generated files.

@improbable-prow-robot improbable-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. labels Apr 17, 2021
@awbraunstein awbraunstein changed the base branch from proto3-optional to master April 17, 2021 02:06
@awbraunstein
Copy link
Contributor Author

@MarcusLongmuir would appreciate a review on this. Thanks.

@MarcusLongmuir
Copy link
Contributor

Thanks for raising this @awbraunstein. @moadz is currently reviewing this. Will try to cut a release for this soon after it lands.

Copy link

@moadz moadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for completing this!

@improbable-prow-robot improbable-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2021
@improbable-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: moadz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@improbable-prow-robot improbable-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2021
@MarcusLongmuir MarcusLongmuir merged commit f8a4adb into improbable-eng:master Apr 22, 2021
@stof
Copy link

stof commented Apr 22, 2021

@MarcusLongmuir any chance to make a release containing this feature ?

protoc 3.15.0 ships with it as a non-experimental feature.

@awbraunstein awbraunstein deleted the proto3-optional branch April 22, 2021 18:27
sxlijin added a commit to sxlijin/rules_typescript_proto that referenced this pull request Mar 9, 2022
This was implemented in ts-protoc-gen in
improbable-eng/ts-protoc-gen#275 and released
nearly a year ago in [email protected]; glancing through the issue
history it looks like the only reason this hasn't been bumped yet is
because no one's tried to contribute it.
sxlijin added a commit to sxlijin/rules_typescript_proto that referenced this pull request Mar 9, 2022
This was implemented in ts-protoc-gen in
improbable-eng/ts-protoc-gen#275 and released
nearly a year ago in [email protected]; glancing through the issue
history it looks like the only reason this hasn't been bumped yet is
because no one's tried to contribute it.
Dig-Doug pushed a commit to Dig-Doug/rules_typescript_proto that referenced this pull request Mar 10, 2022
This was implemented in ts-protoc-gen in
improbable-eng/ts-protoc-gen#275 and released
nearly a year ago in [email protected]; glancing through the issue
history it looks like the only reason this hasn't been bumped yet is
because no one's tried to contribute it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 600+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for proto3 optional fields
5 participants