code review 61180043: math/big: add support for general encoding interfaces

math/big: add support for general encoding interfaces TextMarshaller and TextUnmarshaller to ease transport of unlimited precision rational numbers. Fixes issue 7287. Consists of encode and decode functions and two test functions, one using JSON and one using XML. Each verifies round trips for integers (rationals with denominator == 1) and for fractional vaues.

[cookieo9, 2014-02-09 01:03:57]

https://codereview.appspot.com/61180043/diff/10002/src/pkg/math/big/rat.go File src/pkg/math/big/rat.go (right): https://codereview.appspot.com/61180043/diff/10002/src/pkg/math/big/rat.go#ne... src/pkg/math/big/rat.go:590: func (x *Rat) MarshalJSON() ([]byte, error) { Considering the implementation, wouldn't it make more sense to implement encoding.TextMarshaller/TextUnmarshaller? http://golang.org/pkg/encoding/#TextMarshaler

[mtj, 2014-02-09 01:47:34]

On 2014/02/09 01:03:57, cookieo9 wrote: > https://codereview.appspot.com/61180043/diff/10002/src/pkg/math/big/rat.go > File src/pkg/math/big/rat.go (right): > > https://codereview.appspot.com/61180043/diff/10002/src/pkg/math/big/rat.go#ne... > src/pkg/math/big/rat.go:590: func (x *Rat) MarshalJSON() ([]byte, error) { > Considering the implementation, wouldn't it make more sense to implement > encoding.TextMarshaller/TextUnmarshaller? > > http://golang.org/pkg/encoding/#TextMarshaler Yes! You are right. I simply thought to replicate what was in Int into Rat, but they're both handled more generally by supporting the TextMarshaller path. Will reconsider. Excellent point, and who knows if someone may wish to hex encode or use other if the encoding tools.

[mtj, 2014-02-09 01:51:48]

Hello gri@golang.org, cookieo9@gmail.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go

[bradfitz, 2014-02-09 02:01:01]

s/lacked/add/ so the summary doesn't sound so weak and weird. Or better: "add JSON support to Rat" On Feb 8, 2014 5:51 PM, <michael.jones@gmail.com> wrote: > Hello gri@golang.org, cookieo9@gmail.com (cc: > golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > https://codereview.appspot.com/61180043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > 

[mtj1, 2014-02-09 02:06:21]

I'm taking the "ditch JSON" and "add TextMarshaller" comment to heart. I think we can't just do that only, because the existing functions are exposed in the Int API... On Sat, Feb 8, 2014 at 6:01 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > s/lacked/add/ so the summary doesn't sound so weak and weird. > > Or better: "add JSON support to Rat" > On Feb 8, 2014 5:51 PM, <michael.jones@gmail.com> wrote: > >> Hello gri@golang.org, cookieo9@gmail.com (cc: >> golang-codereviews@googlegroups.com), >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> https://codereview.appspot.com/61180043/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "golang-codereviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-codereviews+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/groups/opt_out. >> > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > -- *Michael T. Jones | Chief Technology Advocate | mtj@google.com <mtj@google.com> | +1 650-335-5765* 

[mtj, 2014-02-09 03:46:44]

Hello gri@golang.org, cookieo9@gmail.com, bradfitz@golang.org, mtj@google.com (cc: golang-codereviews@googlegroups.com), Please take another look.

[gri, 2014-02-10 19:48:15]

Please also update the CL description and add, just so: Fixes issue 7287. Thanks. https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat.go File src/pkg/math/big/rat.go (right): https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat.go#ne... src/pkg/math/big/rat.go:596: _, ok := r.SetString(string(text)) if _, ok := r.SetString(string(text)); !ok { ... } https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat.go#ne... src/pkg/math/big/rat.go:598: return fmt.Errorf("math/big: cannot unmarshal %s into a *big.Rat", text) s/%s/%q/ so we can see special chars in case of an error https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.go File src/pkg/math/big/rat_test.go (right): https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.... src/pkg/math/big/rat_test.go:438: var ratDenom = []string{ s/ratDenom/ratDenoms/ (we usually use the plural for lists of test data in this file) https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.... src/pkg/math/big/rat_test.go:455: t.Errorf("marshaling of %s failed: %s", &tx, err) add: continue (we can't unmarshal if marshalling failed) https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.... src/pkg/math/big/rat_test.go:459: t.Errorf("unmarshaling of %s failed: %s", &tx, err) add: continue https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.... src/pkg/math/big/rat_test.go:475: t.Errorf("marshaling of %s failed: %s", &tx, err) continue https://codereview.appspot.com/61180043/diff/80001/src/pkg/math/big/rat_test.... src/pkg/math/big/rat_test.go:479: t.Errorf("unmarshaling of %s failed: %s", &tx, err) continue

[mtj, 2014-02-12 04:48:10]

Hello gri@golang.org, cookieo9@gmail.com, bradfitz@golang.org, mtj@google.com (cc: golang-codereviews@googlegroups.com), Please take another look.

[gri, 2014-02-13 04:11:10]

LGTM Thanks.

[gri, 2014-02-13 04:15:54]

Actually, can you please hg gofmt or gofmt -w $GOROOT/src/pkg/math/big/ and re-upload? Also, please hg change the CL description to contain the "Fixes issue 7287." on a separate line, by itself (I'm not sure it is accepted when on the same line with other text). (I could do this myself but then you're local repo and the submitted version differ and you may get merge unanticipated merge conflicts). Thanks! - gri On Wed, Feb 12, 2014 at 8:11 PM, <gri@golang.org> wrote: > LGTM > > Thanks. > > https://codereview.appspot.com/61180043/ > 

[mtj, 2014-02-13 06:25:55]

Hello gri@golang.org, cookieo9@gmail.com, bradfitz@golang.org, mtj@google.com (cc: golang-codereviews@googlegroups.com), Please take another look.

[gri, 2014-02-13 16:41:47]

LGTM Thanks.

[gri, 2014-02-13 16:42:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=20f12ba1f1fb *** math/big: add support for general encoding interfaces TextMarshaller and TextUnmarshaller to ease transport of unlimited precision rational numbers. Fixes issue 7287. Consists of encode and decode functions and two test functions, one using JSON and one using XML. Each verifies round trips for integers (rationals with denominator == 1) and for fractional vaues. LGTM=gri R=gri, cookieo9, bradfitz, mtj CC=golang-codereviews https://codereview.appspot.com/61180043 Committer: Robert Griesemer <gri@golang.org>

[gobot, 2014-02-13 16:57:01]

This CL appears to have broken the openbsd-amd64-rootbsd builder.