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

Support IsZeroers when checking for isZero #353

Closed
wants to merge 1 commit into from

Conversation

RaMin0
Copy link

@RaMin0 RaMin0 commented Apr 24, 2022

Expanding the capabilities of omitzero by allowing to check if the value implements the isZeroer interface below:

type isZeroer interface {
  IsZero() bool
}

This enables values, such as of the type time.Time, to be treated as expected when they have a zero value.

@arp242
Copy link
Collaborator

arp242 commented May 27, 2022

Do you know if there is any precedent for the IsZero() method? I know time.Time uses it, but that's the only type (stdlib or commonly used package) I can think of off-hand.

@RaMin0
Copy link
Author

RaMin0 commented May 27, 2022

@arp242 Not really, I also only know about time.Time's. But given the name of the tag field omitzero, and given that this is Go's standard way of handling such cases, I thought adding this capability with an isZeroer interface would expand the zero checking abilities to accommodate for similar cases to mine, what do you think?

@arp242
Copy link
Collaborator

arp242 commented May 27, 2022

One potential issue with this is compatibility: what if people already use a type (time.Time or anything else) that has IsZero() bool that uses omitzero? Right now that does nothing, but with this change it will start eliding those fields. This came up on the Go repo recently and it seems like a common type of error (go vet in 1.19 will warn about it). I once broke an API by adding omitempty somewhere: it should have been there from the start, but it wasn't and when I added it it broke some of our users as their code always expected this field to be there. Had to roll that back in a hurry after some panicky support messages 😅

There's a discussion on the Go issue about adding this to stdlib encoders; I didn't read through it all yet, but if that goes through (eventually...) we'll be "stuck" with a different implementation than stdlib; not that stdlib is always right or the best way, but I find it confusing if it's different here especially for struct tags (which are really just "comments"); also see omitzero/omitempty we have now (added before Go's encoders used I think) which I even find confusing myself!


So those are my concerns: compatibility and being different from stdlib. Compatibility can be fixed by adding this to v2, stdlib compatibility is harder, but also not strictly a deal-breaker (just unfortunate).

I do think this is a problem worth addressing though, either with this solution or with something else. It might be worth to check what other libraries (yaml, hson, whatnot) do with this, if anything. Maybe they have a better idea/implementation? I also need to read through that issue 11939 in full because there might be some good comments there.

PS. also note that the omitzero tag will probably be removed in v2 of this library in favour of just using omitempty similar to how the stdlib encoders work, so if this will be in v2 we need to consider that too.

@arp242 arp242 added v2 and removed v2 labels May 27, 2022
arp242 added a commit that referenced this pull request May 29, 2022
A common use case for this is time.Time, which will get marshaled
otherwise.

A struct with private fields set is *not* considered to be "empty"; not
sure yet if this is the best behaviour 🤔 It is the easiest to implement
though.

Fixes #353
@arp242
Copy link
Collaborator

arp242 commented May 29, 2022

Possible alternative: #356

This way you don't need to add an IsZero() method everywhere, and backward compatibility issues are also not as big of a deal.

Downside is that an "empty struct" is not an easy question; is a struct "empty" when all fields are the zero value, including private fields? Or only exported fields? Need to think about this for a bit, but overall I think that's a better approach to solve this issue.

arp242 added a commit that referenced this pull request Jun 7, 2022
A common use case for this is time.Time, which will get marshaled
otherwise.

A struct with private fields set is *not* considered to be "empty"; not
sure yet if this is the best behaviour 🤔 It is the easiest to implement
though.

Fixes #353
arp242 added a commit that referenced this pull request Jun 7, 2022
A common use case for this is time.Time, which will get marshaled
otherwise.

A struct with private fields set is *not* considered to be "empty"; this
is because some structs (such as time.Time) contain only private fields.

Fixes #353
@arp242 arp242 closed this in #356 Jun 7, 2022
@arp242
Copy link
Collaborator

arp242 commented Jun 7, 2022

Fixed/implemented via #356.

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