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

V2: Make DescField.presence available for all fields #822

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

timostamm
Copy link
Member

The property DescField.presence tells whether a field tracks presence, but only for singular fields. This change makes it available for all fields. The values match with the documentation, which should be helpful for users who want to take a closer look at the schema.

The property is also available for extension descriptors. It's not well documented, but implicit presence (e.g. a proto3 singular scalar field) does not apply to extensions. Otherwise, setting an extension value 0 would be ambigous.

Comment on lines +707 to +708
describe("JsonWriteOptions", () => {
describe("emitDefaultValues", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding coverage because the logic for deciding when to emit changed.

Comment on lines +142 to +143
if (!opts.emitDefaultValues || f.presence !== IMPLICIT) {
// Fields with implicit presence omit zero values (e.g. empty string) by default
Copy link
Member Author

Choose a reason for hiding this comment

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

The function canEmitFieldDefaultValue is replaced by this simple check, matching the specified behavior:

Fields that don’t support presence and that have their default value are omitted by default in JSON output (for example, an implicit presence integer with a 0 value, implicit presence string fields that are empty strings, and empty repeated and map fields).

We might want to rename the serialization option emitDefaultValues to emitFieldsWithoutPresence as a follow-up.

@timostamm
Copy link
Member Author

@srikrsna-buf, the presence and other properties of DescField/DescExtension are computed eagerly (that wasn't the case some time ago), and I can't think of a way that the kludge we use in the extension container should cause any more problems. So I think we should keep the extension container.

@timostamm timostamm marked this pull request as ready for review April 30, 2024 13:57
@timostamm timostamm merged commit 0ae7c61 into v2 Apr 30, 2024
7 checks passed
@timostamm timostamm deleted the tstamm/desc-field-presence-for-all branch April 30, 2024 16:30
@timostamm timostamm changed the title Make DescField.presence available for all fields V2: Make DescField.presence available for all fields Apr 30, 2024
@timostamm timostamm mentioned this pull request May 13, 2024
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