code review 4625081: json: allow using '$' and '-' as the struct field's tag

json: allow using '$' and '-' as the struct field's tag

[mikio, 2011-06-30 22:27:14]

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

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

[rsc, 2011-06-30 22:35:42]

I was thinking that now that there's an explicit json:
required we might as well accept any tag that can
be a valid json string constant.

[bradfitz, 2011-06-30 22:39:34]

Please add a test.  Go makes it too easy.  No excuse not to.

Failure to add a test will result in my intentionally trying to regress your
patch in a future patch of my own.  :-)

On Thu, Jun 30, 2011 at 3:27 PM, <mikioh.mikioh@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> json: enable hyphen as a valid tag character
>
> Please review this at
http://codereview.appspot.com/**4625081/<http://codereview.appspot.com/4625081/>
>
> Affected files:
>  M src/pkg/json/encode.go
>
>
> Index: src/pkg/json/encode.go
> ==============================**==============================**=======
> --- a/src/pkg/json/encode.go
> +++ b/src/pkg/json/encode.go
> @@ -40,7 +40,7 @@
>  // becomes a member of the object.  By default the object's key string
>  // is the struct field name.  If the struct field's tag has a "json" key
> with a
>  // value that is a non-empty string consisting of only Unicode letters,
> -// digits, and underscores, that value will be used as the object key.
> +// digits, hyphens, and underscores, that value will be used as the object
> key.
>  // For example, the field tag `json:"myName"` says to use "myName"
>  // as the object key.
>  //
> @@ -316,7 +316,7 @@
>                return false
>        }
>        for _, c := range s {
> -               if c != '_' && !unicode.IsLetter(c) && !unicode.IsDigit(c)
> {
> +               if c != '_' && c != '-' && !unicode.IsLetter(c) &&
> !unicode.IsDigit(c) {
>                        return false
>                }
>        }
>
>
>

[mikio, 2011-07-01 03:23:13]

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

Please take another look.

[mattn, 2011-07-01 03:44:15]

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

http://codereview.appspot.com/4625081/diff/11002/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:324: if unicode.IsLetter(c) || unicode.IsDigit(c) {
original code return true if never find invalid characters.
but new code return true when find first valid character.
When first characetr is valid, and that include invalid character, this function
return true. is this a ok?

i'm not sure, i don't know why this CL is needed, this change seems to be
broken.

[mikio, 2011-07-01 03:47:08]

On Fri, Jul 1, 2011 at 12:44 PM,  <mattn.jp@gmail.com> wrote:

> original code return true if never find invalid characters.
> but new code return true when find first valid character.

oops, thx.

[mikio, 2011-07-01 04:13:03]

Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org,
mattn.jp@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2011-07-01 04:17:37]

Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org,
mattn.jp@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-07-01 04:20:52]

LGTM

test passed.

On 2011/07/01 04:17:37, mikioh wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:rsc@golang.org,
mailto:bradfitz@golang.org,
> mailto:mattn.jp@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[mikio, 2011-07-01 07:25:27]

Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org,
mattn.jp@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-07-01 14:30:54]

Handing off to adg.

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

http://codereview.appspot.com/4625081/diff/5003/src/pkg/json/encode.go#newcode41
src/pkg/json/encode.go:41: // is the struct field name.  If the struct field's
tag has a "json" key with a
I really think this should be

If the struct field's tag has a "json" key, its value will be used as the object
key.

with no restrictions on the key, since it is after all a quoted string.
The restrictions were introduced to avoid misusing proto buffer
tags as json tags.  We have a better story for that now.

[mikio, 2011-07-02 10:23:28]

On Fri, Jul 1, 2011 at 11:30 PM,  <rsc@golang.org> wrote:

> If the struct field's tag has a "json" key, its value will be used as
> the object key.
>
> with no restrictions on the key, since it is after all a quoted string.
> The restrictions were introduced to avoid misusing proto buffer
> tags as json tags.  We have a better story for that now.

Yeah, that makes sense. will try.

[mikio, 2011-07-05 03:17:32]

Hello adg@golang.org, rsc@golang.org, bradfitz@golang.org, mattn.jp@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2011-07-08 00:47:18]

Ping.

On Tue, Jul 5, 2011 at 12:17 PM,  <mikioh.mikioh@gmail.com> wrote:

> Hello adg@golang.org, rsc@golang.org, bradfitz@golang.org,
> mattn.jp@gmail.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4625081/

[mikio, 2011-07-13 02:42:52]

Ping.

On Fri, Jul 8, 2011 at 9:47 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> Ping.
>
> On Tue, Jul 5, 2011 at 12:17 PM,  <mikioh.mikioh@gmail.com> wrote:
>
>> Hello adg@golang.org, rsc@golang.org, bradfitz@golang.org,
>> mattn.jp@gmail.com (cc: golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>> http://codereview.appspot.com/4625081/

[adg, 2011-07-13 04:52:06]

http://codereview.appspot.com/4625081/diff/4004/src/pkg/json/tagkey_test.go
File src/pkg/json/tagkey_test.go (right):

http://codereview.appspot.com/4625081/diff/4004/src/pkg/json/tagkey_test.go#n...
src/pkg/json/tagkey_test.go:80: case " !\"#$%&'()*+,-./", "0123456789:;<=>?",
"@ABCDEFGHIJKLMO", "PQRSTUVWXYZ[\\]^_", "`abcdefghijklmno",
"pqrstuvwxyz{|}~\x7f", "いろはにほへと":
This is a bit messy. Add the key name to the test cases so we can just look for
the specific key/value pair.

[rsc, 2011-07-13 04:53:36]

Sorry, I have been leaving this to the side
while we hammer out what is going to
happen with Brad's 'omit zero values' proposal.
That had some bearing on whether we accept
absolutely anything here.

[mikio, 2011-07-13 05:56:47]

http://codereview.appspot.com/4625081/diff/4004/src/pkg/json/tagkey_test.go
File src/pkg/json/tagkey_test.go (right):

http://codereview.appspot.com/4625081/diff/4004/src/pkg/json/tagkey_test.go#n...
src/pkg/json/tagkey_test.go:80: case " !\"#$%&'()*+,-./", "0123456789:;<=>?",
"@ABCDEFGHIJKLMO", "PQRSTUVWXYZ[\\]^_", "`abcdefghijklmno",
"pqrstuvwxyz{|}~\x7f", "いろはにほへと":
On 2011/07/13 04:52:06, adg wrote:
> This is a bit messy. Add the key name to the test cases so we can just look
for
> the specific key/value pair.

Done.

[mikio, 2011-07-13 05:57:09]

Hello adg@golang.org, rsc@golang.org, bradfitz@golang.org, mattn.jp@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-07-13 21:24:02]

We need a way to give a few options to the json: tag
besides the key name.  Instead of allowing absolutely
anything here, I propose to make the tag value a ,-separated
list of things, the first of which is the key name.
That would mean you cannot use , in a key name
with Go's json package even though you can in JSON.

Does that solve your original problem?

Russ

[gustavo_niemeyer.net, 2011-07-13 22:04:47]

> We need a way to give a few options to the json: tag
> besides the key name.  Instead of allowing absolutely
> anything here, I propose to make the tag value a ,-separated
> list of things, the first of which is the key name.
> That would mean you cannot use , in a key name
> with Go's json package even though you can in JSON.

FWIW, I'm using "/" right now in gobson/mgo to separate flags from key
name, in a similar way you suggest.  I'm still holding the changes to
tag handling (which means it's broken with the latest weekly), and
would be happy to use a common convention there.  Switching to ,
sounds reasonable to me.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[bradfitz, 2011-07-13 22:11:00]

On Wed, Jul 13, 2011 at 3:04 PM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> > We need a way to give a few options to the json: tag
> > besides the key name.  Instead of allowing absolutely
> > anything here, I propose to make the tag value a ,-separated
> > list of things, the first of which is the key name.
> > That would mean you cannot use , in a key name
> > with Go's json package even though you can in JSON.
>
> FWIW, I'm using "/" right now in gobson/mgo to separate flags from key
> name, in a similar way you suggest.  I'm still holding the changes to
> tag handling (which means it's broken with the latest weekly), and
> would be happy to use a common convention there.  Switching to ,
> sounds reasonable to me.
>

protobuf picked "," first, so json would follow that for consistency.

[mikio, 2011-07-13 23:11:51]

Yes, what I just want is parsing  "media-types" like hyp-t'd tag/key.

On Jul 14, 2011, at 6:23 AM, Russ Cox <rsc@golang.org> wrote:

> We need a way to give a few options to the json: tag
> besides the key name.  Instead of allowing absolutely
> anything here, I propose to make the tag value a ,-separated
> list of things, the first of which is the key name.
> That would mean you cannot use , in a key name
> with Go's json package even though you can in JSON.
> 
> Does that solve your original problem?
> 
> Russ

[rsc, 2011-07-13 23:14:06]

Okay.  Let's keep the current restrictions as is,
then, and just expand the name set to allow - and $,
rather than do everything but comma.

Russ

[mikio, 2011-07-14 00:39:32]

Hello adg@golang.org, rsc@golang.org, bradfitz@golang.org, mattn.jp@gmail.com,
gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-07-14 00:41:23]

LGTM

[rsc, 2011-07-14 00:41:36]

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

json: allow using '$' and '-' as the struct field's tag

R=adg, rsc, bradfitz, mattn.jp, gustavo
CC=golang-dev
http://codereview.appspot.com/4625081

Committer: Russ Cox <rsc@golang.org>