testify icon indicating copy to clipboard operation
testify copied to clipboard

document that encoding/json omitzero/IsZero is not about assert.IsZero/reflect.Zero

Open ccoVeille opened this issue 5 months ago • 2 comments

Description

The addition of omitzero to encoding/json creates confusion

Proposed solution

  • Add information about this to the documentation of assert.Zero, or somewhere else more appropriate
  • add a unit test with a simple struct that will implement IsZero() bool and return true to validate no changes is done in that direction

Use case

Prevent confusion and PR that might pop out

  • https://github.com/stretchr/testify/pull/1704

ccoVeille avatar Jun 01 '25 19:06 ccoVeille

I don't understand what should be added to the documentation. How are you confused?

If, as a maintainer, I don't understand the task to do, I don't expect an external contributor to understand better. So the labels good-first-issue and help-wanted are not appropriate.

dolmen avatar Jun 10 '25 13:06 dolmen

Assigned to @ccoVeille : update the description to describe in depth your confusion. Or maybe just write the PR.

dolmen avatar Jun 10 '25 13:06 dolmen

testify:

Zero asserts that i is the zero value for its type.

NotZero asserts that i is not the zero value for its type.

std:

The "omitzero" option specifies that the field should be omitted from the encoding if the field has a zero value, according to rules:

  1. If the field type has an "IsZero() bool" method, that will be used to determine whether the value is zero.

  2. Otherwise, the value is zero if it is the zero value for its type.

We're certainly concise, we say the same thing as clause 2 from the standard library.

I don't think the example in #1704 is someone being confused about what zero means, more about what we should consider to be zero, which I think we should not change.

But your experience is valid if you think people are confused. Best place to discuss a documentation change would be a PR.

I'd also be happy to accept a new test to fix in place our current behaviour of ignoring IsZero methods.

brackendawson avatar Aug 06 '25 19:08 brackendawson