code review 5067044: xml: Fix inconsistant XMLName behaviour for xml.Marshal...

xml: Fix inconsistant XMLName behaviour for xml.Marshal/Unmarshal

When xml.Marshal is called on a struct it will happily reflect the information in the "tag" of an XMLName member regardless of the type to give the struct a tag-name in it's XML form. This is backed up by the documentation which says:

However xml.Unmarshal *does* care about the XMLName field being of type xml.Name, and currently returns the error "field XMLName does not have type xml.Name" if you have it set to something else.

This is firstly inconsistant with xml.Marshal but it also makes it impossible to use xml.Marshal alongside other Marshallers (like json/bson) without poluting the state's namespace with XMLName fields. Inorder to exclude fields from other Marshallers the convention has been started to tag fields as "omitempty"; which will cause the field not to display if it is at it's "zero" state, XMLName cannot have such as zero-state since it is a struct, so it is nicer to use a pointer/bool value for XMLName so it can be easily excluded when I want to Marshal my struct by some other wire format.

Attached is the proposed minor change, that simply stops erring if it can't set the name on the XMLName field, which is just optional metadata anyway.
Fixes issue 2265.

[chrisfarms, 2011-09-20 11:53:39]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[kevlar, 2011-09-20 19:04:52]

LGTM

I would update the description, though, to be something like:

xml: only write to XMLName if it's a Name in Unmarshal

Currently, Unmarshal returns an error, but Marshal() simply ignores it.  This
change makes the behavior consistent.

[kevlar, 2011-09-20 19:06:47]

Oh, you might also add a test or two.

[chrisfarms, 2011-10-11 20:55:36]

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

Please take another look.

[chrisfarms, 2011-10-11 20:57:07]

sorry didn't add the test file ... *quickly learns process*.. brb

[chrisfarms, 2011-10-11 21:00:23]

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

Please take another look.

[nigeltao, 2011-10-13 03:03:42]

Setting rsc as the reviewer.

[rsc, 2011-10-14 21:28:20]

LGTM

[rsc, 2011-10-14 21:29:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=94bf75f51cbe ***

xml: match Marshal's XMLName behavior in Unmarshal

When xml.Marshal is called on a struct it will happily
reflect the information in the "tag" of an XMLName member
regardless of the type to give the struct a tag-name in
it's XML form. This is backed up by the documentation which
says:

However xml.Unmarshal *does* care about the XMLName field
being of type xml.Name, and currently returns the error
"field XMLName does not have type xml.Name" if you have it
set to something else.

This is firstly inconsistant with xml.Marshal but it also
makes it impossible to use xml.Marshal alongside other
Marshallers (like json/bson) without poluting the state's
namespace with XMLName fields. Inorder to exclude fields
from other Marshallers the convention has been started to
tag fields as "omitempty"; which will cause the field not
to display if it is at it's "zero" state, XMLName cannot
have such as zero-state since it is a struct, so it is nicer
to use a pointer/bool value for XMLName so it can be easily
excluded when I want to Marshal my struct by some other
wire format.

Attached is the proposed minor change, that simply stops
erring if it can't set the name on the XMLName field, which
is just optional metadata anyway.
Fixes issue 2265.

R=rsc
CC=golang-dev
http://codereview.appspot.com/5067044

Committer: Russ Cox <rsc@golang.org>