code review 4528114: Package xml: Add ability to use non-string fields as at...

Package xml: Add ability to use non-string fields as attributes.

[ushakov, 2011-06-01 18:54:09]

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-06-01 20:47:01]

I have mild objections to this.  This seems to set the precedent that we can
allow any XML PCDATA to be stored in any compatible data type, which adds a lot
of processing up-front when you parse the XML data structure that may never get
used, and XML unmarshal is already relatively intensive.  I won't stand in your
way though.

If you do go this route, I think you shouldn't go half-way: make it possible for
"chardata" attributes to also be non-strings as well.

Then you can do <answer>42</answer> and have it interpreted as an int.

I would also like to see some discussion about non-string attributes in the
Unmarshal documentation.  There's no point implementing it if it's not
documented.

[ushakov, 2011-06-02 17:29:34]

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

Please take another look.

[ushakov, 2011-06-02 17:30:58]

Hi

I'm sorry I don't fully understand your objections. Current implementation
already allows for <answer>42</answer> to be unmarshalled as int (see docs:

Unmarshal maps an XML element to an integer or floating-point field by setting
the field to the result of interpreting the string value in decimal. There is no
check for overflow.

), at http://golang.org/pkg/xml/

I just wanted the same to be available in attrs.

I have slightly updated the documentation to talk about attributes also. What do
you think?

Thanks for review!

M.

[kevlar, 2011-06-02 19:20:13]

My <answer/> example was incomplete.  Take this somewhat more plausible example:

<port>80</port>
<port>
  80
  <ssl version="3">
    <cert>/path/to/cert</cert>
  </ssl>
</port>

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go#newcode120
src/pkg/xml/read.go:120: //      maps the sub-element to that struct field.
Not sure if it's supposed to be undocumented, but something should probably also
be said about the "comment" tag, so you may add it if you feel adventurous.

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go#newcode478
src/pkg/xml/read.go:478: func putData(data []byte, where reflect.Value) (err
os.Error) {
I'm not sure what the standard name for this would be, but I think it might be
more idiomatically named:

func copyValue(dst reflect.Value, src []byte) (err os.Error)

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read_test.go
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read_test.go#newc...
src/pkg/xml/read_test.go:359: </attrtest>
You need some more thorough tests.  Off the top of my head, I know I'd like to
see tests using types whose underlying type is not a string.

[rsc, 2011-06-02 19:31:18]

Using non-string types for attributes seems fine.
I don't see any reason this would make things
inherently more expensive.

Russ

[rsc, 2011-06-03 17:57:34]

LGTM

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go#newcode122
src/pkg/xml/read.go:122: // Unmarshal maps an XML element or attribute value to
a string or
This is weird because it talks about "that element" later.
Maybe undo the change and then add at the end of this paragraph

// Unmarshal maps an attribute value to a string or []byte by saving the value
in the string.

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go#newcode126
src/pkg/xml/read.go:126: // Unmarshal maps an XML element or attribute value to
a slice by
same

[ushakov, 2011-06-03 18:10:55]

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go#newcode120
src/pkg/xml/read.go:120: //      maps the sub-element to that struct field.
On 2011/06/02 19:20:13, kevlar wrote:
> Not sure if it's supposed to be undocumented, but something should probably
also
> be said about the "comment" tag, so you may add it if you feel adventurous.

Done.

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read.go#newcode478
src/pkg/xml/read.go:478: func putData(data []byte, where reflect.Value) (err
os.Error) {
On 2011/06/02 19:20:13, kevlar wrote:
> I'm not sure what the standard name for this would be, but I think it might be
> more idiomatically named:
> 
> func copyValue(dst reflect.Value, src []byte) (err os.Error)

Done.

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read_test.go
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/4528114/diff/9001/src/pkg/xml/read_test.go#newc...
src/pkg/xml/read_test.go:359: </attrtest>
On 2011/06/02 19:20:13, kevlar wrote:
> You need some more thorough tests.  Off the top of my head, I know I'd like to
> see tests using types whose underlying type is not a string.

Not sure I understand you. This test already tests an int, a float, a uint8, and
a bool, which all are not backed by strings. Can you please elaborate what are
you asking for? Thanks!

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go#newcode122
src/pkg/xml/read.go:122: // Unmarshal maps an XML element or attribute value to
a string or
On 2011/06/03 17:57:34, rsc wrote:
> This is weird because it talks about "that element" later.
> Maybe undo the change and then add at the end of this paragraph
> 
> // Unmarshal maps an attribute value to a string or []byte by saving the value
> in the string.
> 

Done.

http://codereview.appspot.com/4528114/diff/14001/src/pkg/xml/read.go#newcode126
src/pkg/xml/read.go:126: // Unmarshal maps an XML element or attribute value to
a slice by
On 2011/06/03 17:57:34, rsc wrote:
> same

Oh, actually we cannot map attributes to non-trivial (i.e. non-[]byte) slices,
so the change is irrelevant here.

[ushakov, 2011-06-07 20:04:29]

Please take another look.

[rsc, 2011-06-14 15:49:47]

LGTM

Thanks.

[rsc, 2011-06-14 15:51:07]

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

xml: handle non-string attribute fields

R=kevlar, rsc
CC=golang-dev
http://codereview.appspot.com/4528114

Committer: Russ Cox <rsc@golang.org>