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.