Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1177)

Issue 4645069: code review 4645069: reflect: support for struct tag use by multiple packages (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 8 months ago by rsc
Modified:
12 years, 8 months ago
Reviewers:
CC:
r, r2, dsymonds, bradfitz, kevlar, fvb, niemeyer, golang-dev
Visibility:
Public.

Description

reflect: support for struct tag use by multiple packages Each package using struct field tags assumes that it is the only package storing data in the tag. This CL adds support in package reflect for sharing tags between multiple packages. In this scheme, the tags must be of the form key:"value" key2:"value2" (raw strings help when writing that tag in Go source). reflect.StructField's Tag field now has type StructTag (a string type), which has method Get(key string) string that returns the associated value. Clients of json and xml will need to be updated. Code that says type T struct { X int "name" } should become type T struct { X int `json:"name"` // or `xml:"name"` } Use govet to identify struct tags that need to be changed to use the new syntax.

Patch Set 1 #

Patch Set 2 : diff -r a1541ae18b69 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r a1541ae18b69 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r 96427f267d49 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r 96427f267d49 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 96427f267d49 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 7 : diff -r 96427f267d49 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r 96427f267d49 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r e2dd614a21df https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 10 : diff -r b3e7f469a443 https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r b3e7f469a443 https://go.googlecode.com/hg/ #

Patch Set 12 : diff -r b3e7f469a443 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 13 : diff -r dd7479dd252a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+293 lines, -184 lines) Patch
M src/cmd/godoc/codewalk.go View 1 2 chunks +4 lines, -4 lines 0 comments Download
M src/cmd/govet/govet.go View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +31 lines, -6 lines 0 comments Download
M src/pkg/asn1/asn1.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/asn1/asn1_test.go View 1 2 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/asn1/marshal.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/asn1/marshal_test.go View 1 1 chunk +5 lines, -5 lines 0 comments Download
M src/pkg/crypto/ocsp/ocsp.go View 1 2 chunks +10 lines, -10 lines 0 comments Download
M src/pkg/crypto/x509/pkix/pkix.go View 1 4 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/crypto/x509/x509.go View 1 2 3 4 5 6 5 chunks +16 lines, -16 lines 0 comments Download
M src/pkg/go/types/testdata/exports.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/json/decode.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/json/decode_test.go View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -4 lines 0 comments Download
M src/pkg/json/encode.go View 1 2 3 4 5 6 7 8 9 2 chunks +9 lines, -7 lines 0 comments Download
M src/pkg/net/dnsmsg.go View 1 18 chunks +23 lines, -23 lines 0 comments Download
M src/pkg/reflect/all_test.go View 1 2 3 4 5 6 7 8 3 chunks +25 lines, -5 lines 0 comments Download
M src/pkg/reflect/type.go View 1 2 3 4 5 6 7 8 9 6 chunks +70 lines, -10 lines 0 comments Download
M src/pkg/rpc/jsonrpc/all_test.go View 1 1 chunk +3 lines, -3 lines 0 comments Download
M src/pkg/rpc/jsonrpc/client.go View 1 2 3 4 5 6 7 2 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/rpc/jsonrpc/server.go View 1 2 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/xml/embed_test.go View 1 4 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/xml/marshal.go View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/xml/marshal_test.go View 1 2 chunks +17 lines, -17 lines 0 comments Download
M src/pkg/xml/read.go View 1 2 3 4 8 chunks +21 lines, -20 lines 0 comments Download
M src/pkg/xml/read_test.go View 1 6 chunks +21 lines, -21 lines 0 comments Download

Messages

Total messages: 29
rsc
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
12 years, 8 months ago (2011-06-29 03:52:42 UTC) #1
rsc
As implemented in this CL, most uses of f.Tag will no longer compile, because f.Tag ...
12 years, 8 months ago (2011-06-29 03:53:48 UTC) #2
r2
On Jun 29, 2011, at 1:53 PM, Russ Cox wrote: > As implemented in this ...
12 years, 8 months ago (2011-06-29 03:57:39 UTC) #3
rsc
Also, none of the documentation is updated outside of reflect, until the design is ironed ...
12 years, 8 months ago (2011-06-29 04:02:24 UTC) #4
r
http://codereview.appspot.com/4645069/diff/5001/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/4645069/diff/5001/src/pkg/reflect/type.go#newcode664 src/pkg/reflect/type.go:664: func (tag StructTag) Index(key string) string { i wonder ...
12 years, 8 months ago (2011-06-29 04:02:48 UTC) #5
rsc
> i agree with everything except the last step. i agree people will reach for ...
12 years, 8 months ago (2011-06-29 04:03:15 UTC) #6
rsc
> i wonder if it should be (string, bool). it feels wrong to be unable ...
12 years, 8 months ago (2011-06-29 04:05:23 UTC) #7
r2
On Jun 29, 2011, at 2:05 PM, Russ Cox wrote: >> i wonder if it ...
12 years, 8 months ago (2011-06-29 04:06:01 UTC) #8
dsymonds
LGTM http://codereview.appspot.com/4645069/diff/5001/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): http://codereview.appspot.com/4645069/diff/5001/src/pkg/reflect/all_test.go#newcode1520 src/pkg/reflect/all_test.go:1520: {`protobuf:"PB(1,2)" json:"name"`, `json`, `name`}, add one more test ...
12 years, 8 months ago (2011-06-29 04:08:14 UTC) #9
r
i like this a lot. i like how the tags are now much more informative. ...
12 years, 8 months ago (2011-06-29 04:10:58 UTC) #10
bradfitz
On Tue, Jun 28, 2011 at 9:10 PM, <r@golang.org> wrote: > i like this a ...
12 years, 8 months ago (2011-06-29 04:12:56 UTC) #11
r2
On Jun 29, 2011, at 2:12 PM, Brad Fitzpatrick wrote: > On Tue, Jun 28, ...
12 years, 8 months ago (2011-06-29 04:13:39 UTC) #12
rsc
i chose Index to fit with the method names used by reflect. Get is overloaded; ...
12 years, 8 months ago (2011-06-29 04:14:59 UTC) #13
bradfitz
On Tue, Jun 28, 2011 at 9:05 PM, Russ Cox <rsc@golang.org> wrote: > > i ...
12 years, 8 months ago (2011-06-29 04:15:36 UTC) #14
dsymonds
LGTM modulo extra test cases. http://codereview.appspot.com/4645069/diff/8002/src/pkg/json/decode_test.go File src/pkg/json/decode_test.go (right): http://codereview.appspot.com/4645069/diff/8002/src/pkg/json/decode_test.go#newcode46 src/pkg/json/decode_test.go:46: Z string `x:"@#*%(#@"` maybe ...
12 years, 8 months ago (2011-06-29 04:45:41 UTC) #15
rsc
> http://codereview.appspot.com/4645069/diff/8002/src/pkg/json/decode_test.go#newcode46 > src/pkg/json/decode_test.go:46: Z string `x:"@#*%(#@"` > maybe another that has a json tag ...
12 years, 8 months ago (2011-06-29 04:47:59 UTC) #16
r
http://codereview.appspot.com/4645069/diff/14002/src/cmd/govet/govet.go File src/cmd/govet/govet.go (right): http://codereview.appspot.com/4645069/diff/14002/src/cmd/govet/govet.go#newcode217 src/cmd/govet/govet.go:217: } still no test here as far as i ...
12 years, 8 months ago (2011-06-29 04:58:48 UTC) #17
rsc
> http://codereview.appspot.com/4645069/diff/14002/src/cmd/govet/govet.go#newcode217 > src/cmd/govet/govet.go:217: } > still no test here as far as i can ...
12 years, 8 months ago (2011-06-29 05:02:21 UTC) #18
rsc
other comments done
12 years, 8 months ago (2011-06-29 05:04:32 UTC) #19
r2
On Jun 29, 2011, at 3:02 PM, Russ Cox wrote: >> http://codereview.appspot.com/4645069/diff/14002/src/cmd/govet/govet.go#newcode217 >> src/cmd/govet/govet.go:217: } ...
12 years, 8 months ago (2011-06-29 05:06:13 UTC) #20
rsc
added govet test
12 years, 8 months ago (2011-06-29 05:16:09 UTC) #21
r
LGTM assuming govet complains about the tag
12 years, 8 months ago (2011-06-29 05:17:59 UTC) #22
kevlar
LGTM I've been idly wondering (since about the time I started writing xml.Marshal) what a ...
12 years, 8 months ago (2011-06-29 07:47:33 UTC) #23
fvb
http://codereview.appspot.com/4645069/diff/13003/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/4645069/diff/13003/src/pkg/reflect/type.go#newcode712 src/pkg/reflect/type.go:712: // characters and Go string literal syntax. How about ...
12 years, 8 months ago (2011-06-29 12:30:48 UTC) #24
rsc
People seem happy. Submitting. > The only suggestion I have, which if this idea gets ...
12 years, 8 months ago (2011-06-29 13:36:22 UTC) #25
niemeyer
Nice, thanks for pushing this forward. One idea: http://codereview.appspot.com/4645069/diff/13003/src/pkg/json/decode_test.go File src/pkg/json/decode_test.go (right): http://codereview.appspot.com/4645069/diff/13003/src/pkg/json/decode_test.go#newcode45 src/pkg/json/decode_test.go:45: Y ...
12 years, 8 months ago (2011-06-29 13:43:10 UTC) #26
niemeyer
> Since raw string syntax is already in the language, > I'm not too concerned ...
12 years, 8 months ago (2011-06-29 13:46:48 UTC) #27
rsc
>> Since raw string syntax is already in the language, >> I'm not too concerned ...
12 years, 8 months ago (2011-06-29 13:51:49 UTC) #28
rsc
12 years, 8 months ago (2011-06-29 13:52:43 UTC) #29
*** Submitted as http://code.google.com/p/go/source/detail?r=1dd08cd96bc3 ***

reflect: support for struct tag use by multiple packages

Each package using struct field tags assumes that
it is the only package storing data in the tag.
This CL adds support in package reflect for sharing
tags between multiple packages.  In this scheme, the
tags must be of the form

        key:"value" key2:"value2"

(raw strings help when writing that tag in Go source).

reflect.StructField's Tag field now has type StructTag
(a string type), which has method Get(key string) string
that returns the associated value.

Clients of json and xml will need to be updated.
Code that says

        type T struct {
                X int "name"
        }

should become

        type T struct {
                X int `json:"name"`  // or `xml:"name"`
        }

Use govet to identify struct tags that need to be changed
to use the new syntax.

R=r, r, dsymonds, bradfitz, kevlar, fvbommel, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/4645069
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b