Code review - Issue 12703043: code review 12703043: encoding/json: support encoding.TextMarshaler, encoding...https://codereview.appspot.com/2013-08-14T18:56:10+00:00rietveld
Message from unknown
2013-08-09T19:08:35+00:00rscurn:md5:6b9bf937c732ed983d36eb87a849af30
Message from unknown
2013-08-09T19:09:38+00:00rscurn:md5:3c17c52550ae63cb52b3883e78348f8d
Message from rsc@golang.org
2013-08-09T19:09:44+00:00rscurn:md5:0008fcad94b3076089ff5074adb0164f
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go/
Message from unknown
2013-08-09T19:09:49+00:00rscurn:md5:b366cce915d611e73edc6e60b535920d
Message from bradfitz@golang.org
2013-08-09T20:04:16+00:00bradfitzurn:md5:7021b74114885986c93d6701c64bd435
https://codereview.appspot.com/12703043/diff/6001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):
https://codereview.appspot.com/12703043/diff/6001/src/pkg/encoding/json/encode.go#newcode847
src/pkg/encoding/json/encode.go:847: func (e *encodeState) stringBytes(s []byte) (int, error) {
this is sad, that we have two giant copies of a non-trivial function that don't differ at all.
and that you just fixed a bug in both of them.
at least they're adjacent, but could you add a warning comment above both of them to keep the in sync? or better: add a test that they're always equivalent for a large set of test cases?
Message from unknown
2013-08-14T04:32:21+00:00rscurn:md5:f7c5bfb6d5a272d3ce1891bc35672844
Message from rsc@golang.org
2013-08-14T04:32:27+00:00rscurn:md5:da4464a0c87bd5824731485d26fca066
PTAL
I added a test for the two functions being in sync.
Message from bradfitz@golang.org
2013-08-14T18:04:53+00:00bradfitzurn:md5:cae4d858114dbbe251fb6129b19983d6
LGTM
https://codereview.appspot.com/12703043/diff/10001/src/pkg/encoding/json/decode_test.go
File src/pkg/encoding/json/decode_test.go (right):
https://codereview.appspot.com/12703043/diff/10001/src/pkg/encoding/json/decode_test.go#newcode79
src/pkg/encoding/json/decode_test.go:79: *u = unmarshalerText{true} // All we need to see that UnmarshalJson is called.
s/Json/JSON/
Message from unknown
2013-08-14T18:56:01+00:00rscurn:md5:432b9cd3bf50a359ce5e80ec90fab87b
Message from rsc@golang.org
2013-08-14T18:56:10+00:00rscurn:md5:612dfeae47f998affa0f8a220c6a628e
*** Submitted as https://code.google.com/p/go/source/detail?r=117ed8ab3db0 ***
encoding/json: support encoding.TextMarshaler, encoding.TextUnmarshaler
R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12703043