code review 4709044: json: add omitzero struct tag option

json: add omitempty struct tag option

Fixes issue 2032

[bradfitz, 2011-07-13 02:40:18]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-07-13 02:56:00]

This makes me a little uneasy. How often is this going to come up in
practice? Field renaming is relatively common (esp. since Go enforces
casing on exported fields), but this seems pretty niche, and perhaps
better served just by adding MarshalJSON/UnmarshalJSON methods to the
type in question.


Dave.

[r2, 2011-07-13 05:16:22]

This is unpleasant and also precedent-setting, a bad combination.

-rob

[rsc, 2011-07-13 05:19:40]

On Wed, Jul 13, 2011 at 01:16, Rob 'Commander' Pike <r@google.com> wrote:
> This is unpleasant and also precedent-setting, a bad combination.

Can you elaborate?  What's unpleasant?  Just the x.y tag form
or the semantics of the tag too?  I don't see why the x.y tag is
much different from test.foo as a flag name.  It seems much
nicer to use x.y than to require every x to define its own tag
syntax inside the quoted string.  We're out of quoting levels.

Russ

[r2, 2011-07-13 05:23:07]

On 13/07/2011, at 3:19 PM, Russ Cox wrote:

> On Wed, Jul 13, 2011 at 01:16, Rob 'Commander' Pike <r@google.com> wrote:
>> This is unpleasant and also precedent-setting, a bad combination.
> 
> Can you elaborate?  What's unpleasant?  Just the x.y tag form
> or the semantics of the tag too?  I don't see why the x.y tag is
> much different from test.foo as a flag name.  It seems much
> nicer to use x.y than to require every x to define its own tag
> syntax inside the quoted string.  We're out of quoting levels.

I don't like it, no. I would expect that a package that wanted to add things to
a tag would have a field in the tag defined by the package name, and parse that
as needed.  The protobuf stuff was supposed to set the precedent; this CL is
suggesting another one.  I certainly don't want both.

-rob

[rsc, 2011-07-13 05:24:23]

okay.  i'll circle back with brad tomorrow and
suggest another way.

[bradfitz, 2011-07-14 01:06:15]

Hello rsc@golang.org, dsymonds@golang.org, r@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-07-14 01:11:08]

Hello rsc@golang.org, dsymonds@golang.org, r@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2011-07-14 01:11:23]

I'm personally much happier with that.

[rsc, 2011-07-14 01:19:53]

looks pretty good

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:41: // becomes a member of the object unless the field's
"omitzero" option
Can we call this omitempty?
It's not really zero (you don't check for zero structs for example).
Let's use the same definition exp/template just committed
(godoc exp/template |grep -C 5 empty, look in code for isTruth)

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:275: tag, omitZero := "", false
s/""/f.Name/

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:278: tag = ss[0]
if isValidTag(ss[0]) { tag = ss[0] }

might as well put the logic with the parser

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:294: if tag != "" && isValidTag(tag) {
e.string(tag)

[bradfitz, 2011-07-14 01:32:35]

omitempty makes it sounds like it's only for strings and lists.

b := 0  // empty?

But if that's the wording we use elsewhere, I won't fight it.


On Wed, Jul 13, 2011 at 6:19 PM, <rsc@golang.org> wrote:

> looks pretty good
>
>
>
>
> http://codereview.appspot.com/**4709044/diff/11002/src/pkg/**
>
json/encode.go<http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go>
> File src/pkg/json/encode.go (right):
>
> http://codereview.appspot.com/**4709044/diff/11002/src/pkg/**
>
json/encode.go#newcode41<http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newcode41>
> src/pkg/json/encode.go:41: // becomes a member of the object unless the
> field's "omitzero" option
> Can we call this omitempty?
> It's not really zero (you don't check for zero structs for example).
> Let's use the same definition exp/template just committed
> (godoc exp/template |grep -C 5 empty, look in code for isTruth)
>
> http://codereview.appspot.com/**4709044/diff/11002/src/pkg/**
>
json/encode.go#newcode275<http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newcode275>
> src/pkg/json/encode.go:275: tag, omitZero := "", false
> s/""/f.Name/
>
> http://codereview.appspot.com/**4709044/diff/11002/src/pkg/**
>
json/encode.go#newcode278<http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newcode278>
> src/pkg/json/encode.go:278: tag = ss[0]
> if isValidTag(ss[0]) { tag = ss[0] }
>
> might as well put the logic with the parser
>
> http://codereview.appspot.com/**4709044/diff/11002/src/pkg/**
>
json/encode.go#newcode294<http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newcode294>
> src/pkg/json/encode.go:294: if tag != "" && isValidTag(tag) {
> e.string(tag)
>
>
>
http://codereview.appspot.com/**4709044/<http://codereview.appspot.com/4709044/>
>

[r, 2011-07-14 01:33:41]

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4709044/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:205: return false
maybe return true?

[bradfitz, 2011-07-14 18:35:07]

Hello rsc@golang.org, dsymonds@golang.org, r@google.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-07-14 18:38:30]

LGTM

http://codereview.appspot.com/4709044/diff/14001/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4709044/diff/14001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:42: // specified and it has an empty or zero value.  The
object's default key
unless the field is empty and its tag specifies the "omitempty" option.
The empty values are false, 0, any nil pointer or interface value,
and any array, slice, map, or string of length zero.

[bradfitz, 2011-07-14 18:54:58]

*** Submitted as http://code.google.com/p/go/source/detail?r=fe1828189f20 ***

json: add omitempty struct tag option

Fixes issue 2032

R=rsc, dsymonds, r, r
CC=golang-dev
http://codereview.appspot.com/4709044