code review 12556043: encoding/xml: add, support Unmarshaler interface

encoding/xml: add, support Unmarshaler interface

See golang.org/s/go12xml for design.

[rsc, 2013-08-06 18:50:32]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2013-08-09 15:34:19]

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

Please take another look.

[Dominik Honnef, 2013-08-09 16:21:44]

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/read....
src/pkg/encoding/xml/read.go:159: // Unmarshaler is the interface implemented by
objects that can unmarshal
UnmarshalerAttr

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/xml.g...
src/pkg/encoding/xml/xml.go:483: return nil, errRawToken
Should this error condition be documented?

[kortschak, 2013-08-11 01:31:19]

https://codereview.appspot.com/12556043/diff/18001/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

https://codereview.appspot.com/12556043/diff/18001/src/pkg/encoding/xml/read....
src/pkg/encoding/xml/read.go:214: // so it's likely to be incorrect, but we do
what we're told.
I can see this catching people and no error is returned to point to when they do
it. Why not return an error in this case?

[kortschak, 2013-08-12 01:31:47]

I just ported my ncbi/entrez package over to use on this code and it was a
delight to work with.

[rsc, 2013-08-13 16:24:33]

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/12556043/diff/13001/src/pkg/encoding/xml/xml.g...
src/pkg/encoding/xml/xml.go:483: return nil, errRawToken
On 2013/08/09 16:21:45, Dominik Honnef wrote:
> Should this error condition be documented?

Done.

https://codereview.appspot.com/12556043/diff/18001/src/pkg/encoding/xml/read.go
File src/pkg/encoding/xml/read.go (right):

https://codereview.appspot.com/12556043/diff/18001/src/pkg/encoding/xml/read....
src/pkg/encoding/xml/read.go:214: // so it's likely to be incorrect, but we do
what we're told.
On 2013/08/11 01:31:19, kortschak wrote:
> I can see this catching people and no error is returned to point to when they
do
> it. Why not return an error in this case?

It's possible to use correctly. For example, you might make an unmarshaler out
of a func or a map, or perhaps a struct containing a pointer to the thing to
fill in. All those use cases are fine with a non-pointer receiver. In general
you can't tell the difference. So we do what we're told and hope the code says
what the programmer meant.

[Dominik Honnef, 2013-08-13 17:37:48]

LGTM

Just changed some code to use the Unmarshaler interface and it works great so
far.

[kortschak, 2013-08-13 21:04:49]

LGTM

On 14/08/2013, at 1:54 AM, "rsc@golang.org" <rsc@golang.org> wrote:

> It's possible to use correctly. For example, you might make an
> unmarshaler out of a func or a map, or perhaps a struct containing a
> pointer to the thing to fill in. All those use cases are fine with a
> non-pointer receiver. In general you can't tell the difference. So we do
> what we're told and hope the code says what the programmer meant.

Maybe an example to point to this potential trap?

[rsc, 2013-08-14 04:35:33]

Hello golang-dev@googlegroups.com, dominik.honnef@gmail.com,
dan.kortschak@adelaide.edu.au (cc: golang-dev@googlegroups.com),

Please take another look.

[Dominik Honnef, 2013-08-14 16:28:33]

https://codereview.appspot.com/12556043/diff/28001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/12556043/diff/28001/src/pkg/encoding/xml/xml.g...
src/pkg/encoding/xml/xml.go:1859: func (p *printer) EscapeString(s string) {
Why is EscapeString exported? From what I can tell, there's no way to get to a
*printer from outside the package. Is it part of an interface I'm unaware of?

[rsc, 2013-08-14 17:02:20]

On Wed, Aug 14, 2013 at 12:28 PM, <dominik.honnef@gmail.com> wrote:

>
> https://codereview.appspot.**com/12556043/diff/28001/src/**
>
pkg/encoding/xml/xml.go<https://codereview.appspot.com/12556043/diff/28001/src/pkg/encoding/xml/xml.go>
> File src/pkg/encoding/xml/xml.go (right):
>
> https://codereview.appspot.**com/12556043/diff/28001/src/**
>
pkg/encoding/xml/xml.go#**newcode1859<https://codereview.appspot.com/12556043/diff/28001/src/pkg/encoding/xml/xml.go#newcode1859>
> src/pkg/encoding/xml/xml.go:**1859: func (p *printer) EscapeString(s
> string) {
> Why is EscapeString exported? From what I can tell, there's no way to
> get to a *printer from outside the package. Is it part of an interface
> I'm unaware of?
>

It's only for consistency with the other exported-and-yet-unavailable
methods on printer.

Russ

[rsc, 2013-08-14 18:57:32]

On Tue, Aug 13, 2013 at 5:04 PM, Dan Kortschak <
dan.kortschak@adelaide.edu.au> wrote:

> Maybe an example to point to this potential trap?
>

the potential trap is probably not worth pointing out here. it's true of
any method anywhere that needs to mutate its state. it's something you
learn (use pointer receiver) when learning how to write methods, not
something specific to xml.

i agree that examples would be great. those will have to happen in a
separate CL though.

russ

[rsc, 2013-08-14 18:58:14]

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

encoding/xml: add, support Unmarshaler interface

See golang.org/s/go12xml for design.

R=golang-dev, dominik.honnef, dan.kortschak
CC=golang-dev
https://codereview.appspot.com/12556043