Support omitzero #729
Support omitzero #729
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #729 +/- ##
==========================================
- Coverage 77.94% 77.83% -0.12%
==========================================
Files 22 22
Lines 7998 8056 +58
==========================================
+ Hits 6234 6270 +36
- Misses 1349 1370 +21
- Partials 415 416 +1 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for an "omitzero" behavior for YAML marshaling. Key changes include:
- Updated documentation in yaml.go to describe the "omitzero" tag.
- Introduced an IsOmitZero flag in struct.go and a new OmitZero option in option.go.
- Updated zero value determination logic in encode.go and added comprehensive test cases in encode_test.go.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| yaml.go | Updated comments to document the "omitzero" tag behavior |
| struct.go | Added the IsOmitZero field and handling in struct field processing |
| option.go | Introduced OmitZero option and updated OmitEmpty comments |
| encode_test.go | Added new test cases covering various omitzero scenarios |
| encode.go | Refactored zero value logic by splitting checks for omitzero vs omitempty |
encode.go
Outdated
| @@ -728,7 +729,7 @@ type IsZeroer interface { | |||
| IsZero() bool | |||
| } | |||
|
|
|||
| func (e *Encoder) isZeroValue(v reflect.Value) bool { | |||
| func (e *Encoder) isZeroValueByOmitZero(v reflect.Value) bool { | |||
There was a problem hiding this comment.
Can we find better names for those methods? At least for me it's bit cryptic.
It feels we could have:
isZeroValue(previsZeroValueByOmitZero)isEmptyValue(previsZeroValueByOmitEmptyOption)isLegacyEmptyValue(previsZeroValueByOmitEmptyTag)
There was a problem hiding this comment.
I hope I understood it right what's the goal of each method you wanted to have.
There was a problem hiding this comment.
Thank you. I believe the prefix isZeroValue is likely the cause of the confusion, so I am thinking of renaming it as follows.
- isOmittedByOmitZero
- isOmittedByOmitEmptyOption
- isOmittedByOmitEmptyTag
There was a problem hiding this comment.
So it's not about naming but actual logic - the key question is why omitempty option and omitempty tag has to be a different algorithm? Isn't it quite confusing? Wouldn't it be more consistent if we do:
- one omitzero algorithm (
isZeroValue) - two omitempty algorithm that can be switched (consistently switched for both tag and option) --
isEmptyValueorisLegacyEmptyValue
There was a problem hiding this comment.
Ok, sounds like you specifically want to stick to non consistent tag vs option, lets discuss it in one place -- #729 (comment)
encode.go
Outdated
| if (e.omitZero || sf.IsOmitZero) && e.isZeroValueByOmitZero(fieldValue) { | ||
| // omit encoding by omitzero tag or OmitZero option. | ||
| continue | ||
| } | ||
| if e.omitEmpty && e.isZeroValueByOmitEmptyOption(fieldValue) { | ||
| // omit encoding by OmitEmpty option. | ||
| continue | ||
| } | ||
| if sf.IsOmitEmpty && e.isZeroValueByOmitEmptyTag(fieldValue) { | ||
| // omit encoding by omitempty tag. | ||
| continue |
There was a problem hiding this comment.
Ah, so you wanted to "break" OmitEmpty global option to use "new" omit empty since we didn't release it. This is one way of doing it, but I wonder if it wouldn't be much better to have a separate global option for what omitempty logic user wants. This allows us to have omitempty tag and global option 100% consistent which would be preferrable by many (less surprising). So I would do sth like this:
| if (e.omitZero || sf.IsOmitZero) && e.isZeroValueByOmitZero(fieldValue) { | |
| // omit encoding by omitzero tag or OmitZero option. | |
| continue | |
| } | |
| if e.omitEmpty && e.isZeroValueByOmitEmptyOption(fieldValue) { | |
| // omit encoding by OmitEmpty option. | |
| continue | |
| } | |
| if sf.IsOmitEmpty && e.isZeroValueByOmitEmptyTag(fieldValue) { | |
| // omit encoding by omitempty tag. | |
| continue | |
| if (e.omitZero || sf.IsOmitZero) && e.isZeroValueByOmitZero(fieldValue) { | |
| // omit encoding by omitzero tag or OmitZero option. | |
| continue | |
| } | |
| if (e.omitEmpty || sf.IsOmitEmpty) { | |
| if e.enableNewOmitEmptyLogic && e.isEmptyValue(fieldValue) { | |
| continue | |
| } | |
| if !e.enableNewOmitEmptyLogic && e.isLegacyEmptyValue(fieldValue) { | |
| continue | |
| } |
| // OmitEmpty behaves in the same way as the interpretation of the omitempty tag in the encoding/json library. | ||
| // set on all the fields. | ||
| // In the current implementation, the omitempty tag is not implemented in the same way as encoding/json, | ||
| // so please specify this option if you expect the same behavior. | ||
| func OmitEmpty() EncodeOption { |
There was a problem hiding this comment.
Similar to the comment above, ideally we have 3 global options:
OmitEmpty() // Keep old commentary
OmitZero()
EnableStdOmitEmpty() // If true, std omitempty is used for both OmitEmpty option and tag.
There was a problem hiding this comment.
I do not want to add a new option like EnableStdOmitEmpty() because it would be difficult to maintain if it needs to be deprecated later. While it is true that the difference in behavior between the OmitEmpty option and the omitempty tag can be confusing, I believe it is acceptable since it is documented in the comments and, in practice, most people are not aware of the behavioral differences.
There was a problem hiding this comment.
Got it, thanks. I wonder how open you are to reconsider this decision - I still think different tag vs option is much more difficult to maintain and understand in your codebase.
Let's try to unpack a bit more, if you don't mind, but eventually it's your decision:
I assume our intentions here are the same, that go-yaml features and code are functional, yet easy to use and maintain. Those qualities are important if we want to enable wider adoption of this library in the CNCF ecosystem.
I do not want to add a new option like EnableStdOmitEmpty() because it would be difficult to maintain if it needs to be deprecated later.
Let's say we switch one day to "std" (new) omitempty behaviour for tag as well -- I would argue you still would need an option to "Disable" this and switch back to the old behaviour IMO. As a result nothing needs to be deprecated. To make it smooth instead of EnableStdOmitEmpty that might be weird when you make the switch, we could have OmitEmptyLogic(value OmitEmptyLogicEnum) option.
While it is true that the difference in behavior between the OmitEmpty option and the omitempty tag can be confusing, I believe it is acceptable since it is documented in the comments and, in practice, most people are not aware of the behavioral differences.
People are not aware until something breaks in a hidden way. Our product just last month had almost a production issue because of a bad omitempty handling around backward compatibility micro-services rollout. People want useful and consistent defaults and some control to unblock them if needed. Having different omitempty logic for tag vs global option in my opinion violates all of those user needs:
- usefulness: There is no reason to have different tag vs option logic really, especially from user standpoint. Looks like there are some maintainability reasons, but as I mentioned before, is this really true? We need global option to switch logic back (or forward) anyway, so the same amount of code is needed both ways.
- tag vs option is not consistent and it can create surprises. Imagine you want to see and play with encoding and you put global omitempty option. Now let's say the result is as you expect so you then decide to put more "permanent" tags and the output is totally different. Is this something user would want to step into?
- no control to decide themselves e.g. tag having new logic.
As a result, I would argue it's not acceptable, but that's my own experience maintaining ~30k direct importers Go library. 🙈
There was a problem hiding this comment.
Hmmm, I don't understand why a method to revert the logic is necessary. This is because the current behavior of the omitempty tag can be expressed as a combination of the OmitZero and OmitEmpty options (omitempty tag == OmitEmpty and OmitZero option). Similarly, when specifying it with a tag, you can simply specify two tags like omitempty,omitzero.
Additionally, I also question how much the OmitEmpty option will actually be used. For new users utilizing this option, I believe there won't be an issue as long as the comments and behavior are consistent. This is because if they expect behavior similar to the omitempty tag, they can simply specify the OmitZero option at the same time.
There was a problem hiding this comment.
Hmmm, I don't understand why a method to revert the logic is necessary. This is because the current behavior of the omitempty tag can be expressed as a combination of the OmitZero and OmitEmpty options (omitempty tag == OmitEmpty and OmitZero option). Similarly, when specifying it with a tag, you can simply specify two tags like omitempty,omitzero.
Yes, so the revert is necessary because you are planning to make the tag logic switch in v1 (which I don't recommend). Despite loud release note lines etc, users for sure will be surprised and have production incidents/broken yaml processing because of that breaking change. "Revert" (so option to switch back the logic for omitempty) allows to at least quickly use the latest go-yaml again and not get blocked. You might be surprised how "just" adding omitzero is impossible or takes months-years in many cases (e.g. encoding third party structs). I am also not 100% sure you can assume "current" omitempty logic == union of "std" omitempty and omitzero logic -- there are so many nuances. 🙈
Additionally, I also question how much the OmitEmpty option will actually be used. For new users utilizing this option, I believe there won't be an issue as long as the comments and behavior are consistent. This is because if they expect behavior similar to the omitempty tag, they can simply specify the OmitZero option at the same time.
Yea, global option is new so it can do whatever you are right, but since the above (IMO) logic flag is required anyway, it's a straightforward decision to not make anyones life harder and treat OmitEmpty option and tag the same thing. Imagine the next contributor looking into code and realizing tag and option have different meaning despite the exact same known name.
To me this decision to be a bit slower, have extra switch and consistent omitempty is exponentially cheaper and easier to read use and maintain than having different tag vs option and suddenly at one point the same tag and option and no control over that by a user. Users hate to do more work without notice and current go mod + dependabot tools will simply upgrade go-yaml for them with the omitempty breaking change in a hidden way.
Anyway, sorry for being a pain here, I just really would love to make sure we can use this library, and "our" importers (e.g. Kubernetes) are extremely strict of the stability of (even indirect) dependencies. See our previous discussions 🙈
There was a problem hiding this comment.
Also, one important factor why global option of OmitEmptyLogic(enum) might be preferable -- it might be an amazing feature to have, not a "tech debt"
The prev. YAML lib had this omit empty behaviour without a change for a reason - likely because it was wanted by community. So why not allowing users to choose, since the cost of is trivially small (one option, small function to maintain and bit more docs)
|
After discussing with co-maintainer @shuheiktgw , we have decided to merge this PR as is. The reasons are as follows: Prerequisites
Reasons for the DecisionFor users newly utilizing the If we were to align the behavior of the |
|
Ack, thanks for the detailed explanations.
I wonder why you insist on treating the omitempty tag and option as two different things. Those are exactly the same just on different levels: one is on field level and one is global. Isn't this unnecessary complexity to say on field omitempty is this, on global level it's different thing? Anyway, thanks for progressing quickly on that omitempty aspect. Let's see what community feedback will be. |
|
If this is your claim, then it is clearly incorrect. Global options, by definition, affect all fields, so it is important to align their behavior with that at the field level. |
fix #695