Skip to content

Add omitzero support#644

Merged
fxamacker merged 3 commits intofxamacker:masterfrom
liggitt:omitzero
Mar 26, 2025
Merged

Add omitzero support#644
fxamacker merged 3 commits intofxamacker:masterfrom
liggitt:omitzero

Conversation

@liggitt
Copy link
Copy Markdown
Contributor

@liggitt liggitt commented Mar 25, 2025 •
edited
Loading

Description

PR Was Proposed and Welcomed in Currently Open Issue

Checklist (for code PR only, ignore for docs PR)

  • Include unit tests that cover the new code
  • Pass all unit tests
  • Pass all lint checks in CI (goimports, gosec, staticcheck, etc.)
  • Sign each commit with your real name and email.
    Last line of each commit message should be in this format:
    Signed-off-by: Firstname Lastname firstname.lastname@example.com
  • Certify the Developer's Certificate of Origin 1.1
    (see next section).

Certify the Developer's Certificate of Origin 1.1

  • By marking this item as completed, I certify
    the Developer Certificate of Origin 1.1.
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
660 York Street, Suite 102,
San Francisco, CA 94110 USA

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@liggitt liggitt mentioned this pull request Mar 25, 2025
Closed
@liggitt liggitt mentioned this pull request Mar 25, 2025
Merged
3 tasks
@liggitt liggitt force-pushed the omitzero branch 2 times, most recently from cb99045 to fb5051e Compare March 25, 2025 20:05
@fxamacker
Copy link
Copy Markdown
Owner

fxamacker commented Mar 25, 2025 •
edited
Loading

@liggitt Thanks for contributing this PR!

Can discuss that approach here or in the PR, whichever you prefer.

Moved discussion from issue #643 to this PR.

Honoring the omitzero option unconditionally in the cbor struct tag is straightforward.

Sounds good! If cbor struct tag is specified for omitzero, then we can make it unconditional.

I wasn't sure whether to make honoring the omitzero option in the json tag conditional on Go 1.24+ to match stdlib behavior.

For json tag, let's constrain omitzero to go 1.24+ (when cbor struct tag isn't specified).

The original rationale for supporting json tag is to provide consistent behavior. So when omitzero is specified in json tag (without cbor struct tag), the user should expect both cbor and json to be constrained to go 1.24+.

The Go-version-specific bit could be limited to a single private package boolean, which we can manipulate in tests to simplify maintenance and testing.

Sounds good!

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Mar 25, 2025

All of that sounds good. Will ping here when I have unit test coverage completed and this is ready for review (probably later today or tomorrow)

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Mar 25, 2025

Ok, all pushed... tests went faster than I thought.

I copied existing OmitEmpty tests as a starting point, then added a second commit with adjustments where omitzero differs from omitempty and some additional unit tests. Let me know how you would like that squashed.

@liggitt liggitt changed the title WIP - Add omitzero support Add omitzero support Mar 25, 2025
@liggitt liggitt force-pushed the omitzero branch 2 times, most recently from 827ca06 to cbc35b2 Compare March 25, 2025 21:55
@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Mar 25, 2025

(pushed an update to fix linter suggestions in the unit test)

@fxamacker
Copy link
Copy Markdown
Owner

fxamacker commented Mar 25, 2025

I copied existing OmitEmpty tests as a starting point, then added a second commit with adjustments where omitzero differs from omitempty and some additional unit tests. Let me know how you would like that squashed.

Thanks again for opening this PR!

No need to squash because there's only a few commits and they aren't noisy.

I'll take a closer look tomorrow.

BTW, I'm working on one more item for v2.8.0, so there's a chance I won't be able to release tag this week.

liggitt added 3 commits March 26, 2025 09:55
@liggitt
Add omitzero support
167dc75 
Signed-off-by: Jordan Liggitt <liggitt@google.com>
@liggitt
Copy OmitEmpty tests to OmitZero tests
101dea7 
Signed-off-by: Jordan Liggitt <liggitt@google.com>
@liggitt
Adjust OmitZero tests to zero behavior
69da12b 
Signed-off-by: Jordan Liggitt <liggitt@google.com>
fxamacker
fxamacker approved these changes Mar 26, 2025
View reviewed changes
Copy link
Copy Markdown
Owner

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 Thanks again for contributing this PR!

Before tagging a release, I usually need about a week after the last PR is merged to check for regressions, etc.

Also, there is an unrelated optimization I started looking into, but I'm not sure if it will be included in v2.8.0 (depends on schedule).

How soon do you need v2.8.0 tagged?

encode_test.go
testRoundTrip(t, []roundTripTest{{"default values", v, want}}, em, dm)
}

func TestIsZero(t *testing.T) {
Copy link
Copy Markdown
Owner

@fxamacker fxamacker Mar 26, 2025

Choose a reason for hiding this comment

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

Thanks for adding this test for expected behavior of IsZero() for different scenarios.

doc.go
Comment on lines +117 to +121
The "omitzero" option omits zero values from encoding, matching
[stdlib encoding/json behavior](https://pkg.go.dev/encoding/json#Marshal).
When specified in the `cbor` tag, the option is always honored.
When specified in the `json` tag, the option is honored when building with Go 1.24+.

Copy link
Copy Markdown
Owner

@fxamacker fxamacker Mar 26, 2025

Choose a reason for hiding this comment

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

Thanks for updating docs!

@fxamacker fxamacker added the enhancement New feature or request label Mar 26, 2025
@fxamacker fxamacker added this to the v2.8.0 milestone Mar 26, 2025
@fxamacker fxamacker merged commit d34c8ec into fxamacker:master Mar 26, 2025
20 checks passed
@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Mar 26, 2025 •
edited
Loading

How soon do you need v2.8.0 tagged?

I'm not in a rush, Kubernetes is in code freeze for the next few weeks, so kubernetes/kubernetes#130989 (which is what prompted me to pick this up) wouldn't merge until late April anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fxamacker fxamacker fxamacker approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

v2.8.0

Development

Successfully merging this pull request may close these issues.

feature: Add support for omitzero struct tag

2 participants

@liggitt @fxamacker