code review 179540043: encoding/xml: respect name spaces when encoding

encoding/xml: respect name spaces when encoding

We base the prefixes on xmlns-specified attributes
where reasonable.

Fixes issue 7535.

[rog, 2014-11-28 11:48:59]

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal.go
File src/encoding/xml/marshal.go (right):

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:265: // printerPrefix holds a namespace undo record.
This is a bad name. Suggestions for a better one?

[martin.hilton, 2014-12-01 16:50:09]

LGTM, with some small changes

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal.go
File src/encoding/xml/marshal.go (right):

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:265: // printerPrefix holds a namespace undo record.
On 2014/11/28 11:48:59, rog wrote:
> This is a bad name. Suggestions for a better one?

java calls the nearest thing it has a NamespaceContext
(http://docs.oracle.com/javase/7/docs/api/javax/xml/namespace/NamespaceContext...),
but that represents the full collection of name prefix pairs, however it might
be a reasonable name.

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal_...
File src/encoding/xml/marshal_test.go (right):

https://codereview.appspot.com/179540043/diff/20001/src/encoding/xml/marshal_...
src/encoding/xml/marshal_test.go:1423: }}
Please add a test for the example of "<foo xmlns="uri:bar" xmlns:bar="uri:bar"
bar:baz="foo"><baz>quix</baz></foo>"

[matthew.scott, 2014-12-01 19:05:35]

LGTM with Martin's test change.  Thanks for this.

[martin.hilton, 2014-12-02 10:30:50]

Updates LGTM. I have some really unlikely nitpicks.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal.go
File src/encoding/xml/marshal.go (right):

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:303: } else if attr.Name.Space == "xmlns" &&
attr.Name.Local != "" {
http://www.w3.org/TR/REC-xml-names/#xmlReserved says that the namespace for
xmlns is "http://www.w3.org/2000/xmlns/" and
http://golang.org/pkg/encoding/xml/#Name says that Space is the canonical url.
So I think (strictly speaking) this case should handle the space being
"http://www.w3.org/2000/xmlns/". Of course it will still have to handle "xmlns"
as that's what the decoder returns.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:376: if prefix == "" || !isName([]byte(prefix)) ||
strings.Contains(prefix, ":") {
This could be a bit more intelligent. Some namespaces might be URNs that do not
contain a "/", but I would still like it to pick the parrt after the final ':'.
This is an enhancement though, I doubt it's too important.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:824: if pref := p.prefixForNS(name.Space, isAttr);
pref != "" {
pref is pretty much universally called prefix in the other methods. Maybe for
consistency you could make this one prefix too.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/xml.go
File src/encoding/xml/xml.go (right):

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/xml.go#n...
src/encoding/xml/xml.go:51: return name.Local == "xmlns" || name.Space ==
"xmlns"
Again name.Space == "http://www.w3.org/2000/xmlns/" for completeness.

[roger.peppe1, 2014-12-02 12:13:52]

Hello rogpeppe@gmail.com, martin.hilton@canonical.com,
matthew.scott@canonical.com (cc: golang-codereviews@googlegroups.com),

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

[rog, 2014-12-02 12:14:14]

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal.go
File src/encoding/xml/marshal.go (right):

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:303: } else if attr.Name.Space == "xmlns" &&
attr.Name.Local != "" {
On 2014/12/02 10:30:50, martin.hilton wrote:
> http://www.w3.org/TR/REC-xml-names/#xmlReserved says that the namespace for
> xmlns is "http://www.w3.org/2000/xmlns/" and
> http://golang.org/pkg/encoding/xml/#Name says that Space is the canonical url.
> So I think (strictly speaking) this case should handle the space being
> "http://www.w3.org/2000/xmlns/". Of course it will still have to handle
"xmlns"
> as that's what the decoder returns.

I started to do this, but saw the comment in the existing code:
"(The "http://www.w3.org/2000/xmlns/" name space is also predefined as "xmlns",
but users should not be trying to use that one directly - that's our job.)"

I've gone with the spirit of the original code, but changed the comment on Name
to clarify that the xmlns name space is left in its prefix form.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:376: if prefix == "" || !isName([]byte(prefix)) ||
strings.Contains(prefix, ":") {
On 2014/12/02 10:30:50, martin.hilton wrote:
> This could be a bit more intelligent. Some namespaces might be URNs that do
not
> contain a "/", but I would still like it to pick the parrt after the final
':'.
> This is an enhancement though, I doubt it's too important.

Acknowledged.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/marshal....
src/encoding/xml/marshal.go:824: if pref := p.prefixForNS(name.Space, isAttr);
pref != "" {
On 2014/12/02 10:30:50, martin.hilton wrote:
> pref is pretty much universally called prefix in the other methods. Maybe for
> consistency you could make this one prefix too.

Done.

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/xml.go
File src/encoding/xml/xml.go (right):

https://codereview.appspot.com/179540043/diff/40001/src/encoding/xml/xml.go#n...
src/encoding/xml/xml.go:51: return name.Local == "xmlns" || name.Space ==
"xmlns"
On 2014/12/02 10:30:50, martin.hilton wrote:
> Again name.Space == "http://www.w3.org/2000/xmlns/" for completeness.

As for previous comment.

[gobot, 2014-12-19 05:16:48]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/179540043 is best) in the description in your
new CL.

Thanks very much.