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 proto2 required to generated field info #700

Merged
merged 7 commits into from
Feb 2, 2024

Conversation

timostamm
Copy link
Member

@timostamm timostamm commented Feb 1, 2024

To support field presence going forward, this PR adds a property to the minimal field info we generate.

For a proto2 message that defines required string string_field = 1, generated code changes as follows:

  static readonly fields: FieldList = proto2.util.newFieldList(() => [
-   { no: 1, name: "string_field", kind: "scalar", T: 9 /* ScalarType.STRING */ },
+   { no: 1, name: "string_field", kind: "scalar", T: 9 /* ScalarType.STRING */, req: true },

Previously, required fields were identified by the omission of opt: true. The new property will make it possible to merge the reflection-based serialization logic for proto2 and proto3.

When updating to the next release of @bufbuild/protobuf, it is important to update the plugin @bufbuild/protoc-gen-es as well, and regenerate code. (This PR does not rely on the new property req in generated code yet, but upcoming changes will.)

Before any changes were made, test coverage for field info was added in 202baf0.

This also simplifies the logic to create FieldInfo from DescField. The previous behavior was to create PartialFieldInfo. Since the normalization logic is idempotent, and since we have much better test coverage now, we can create FieldInfo instead.
@timostamm timostamm marked this pull request as ready for review February 1, 2024 14:48
@timostamm timostamm merged commit f76c965 into main Feb 2, 2024
5 checks passed
@timostamm timostamm deleted the tstamm/field-info-required branch February 2, 2024 16:36
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