code review 7350048: encoding/xml: fix unmarshalling tags with namespace

encoding/xml: fix unmarshalling tags with namespace

Add a failing test: TestUnmarshalXmlTagWithNamespace and fixed
unmarshal tags with namespace.

xml.Unmarshal does not unmarshal tags like <sparkle:releaseNotesLink> for
`xml:"sparkel:releaseNotesLink"`.

for example:

        type Item struct {
                        // ...
                        SparkleReleaseNotesLink string `xml:"sparkle:releaseNotesLink"`
        }

The above code does not work.

If we simply write:

        type Item struct {
                        // ...
                        SparkleReleaseNotesLink string `xml:"releaseNotesLink"`
        }

this works, but when encoding XML back, we still need the namespace.
(encoding tags with namespace works fine currently)

In the unmarshal func in read.go, it replaces the namespace name with namespace value, so
that we can't compare the fieldinfo with the token.

I did a minimal approach to solve this:

1. Added a nsResolve map[string]string to resolve namespace quickly and
   didn't change the original behavior of translating namespace name.

2. When the field don't match with the token, check the token.Space
   If the token.Space exists, resolve the namespace value and compare it ("ns:field")
   with the field name.

By doing this, we can marshal the struct into into XML correctly,
The below XML is generated by xml.Unmarshal func (with this fix):

    <channel>
        <item>
            <title>Version 2.0 (2 bugs fixed; 3 new features)</title>
            <sparkle:releaseNotesLink>http://you.com/app/2.0.html</sparkle:releaseNotesLink>
            <enclosure url="http://you.com/app/Your Great App 2.0.zip"
                length="1623481" sparkle:version="2.0"
                sparkle:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh"></enclosure>
        </item>
    </channel>

[c9s, 2013-02-18 17:40:18]

Please take a look

[c9s, 2013-02-19 03:12:44]

Hello golang-dev@googlegroups.com,

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

[c9s, 2013-02-19 03:44:20]

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

Please take another look.

[dave_cheney.net, 2013-02-19 04:07:49]

On 2013/02/19 03:44:20, c9s wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Hello,

I'm not sure how you have done this but this CL is based off a revision,
38271c4ce682 that does not appear in the tree. Can you please hg sync && hg mail
7350048 again.

[c9s, 2013-02-19 04:09:18]

Sorry I guess I had some problem with hg.

let me do that.

On 2013/02/19 04:07:49, dfc wrote:
> On 2013/02/19 03:44:20, c9s wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > Please take another look.
> 
> Hello,
> 
> I'm not sure how you have done this but this CL is based off a revision,
> 38271c4ce682 that does not appear in the tree. Can you please hg sync && hg
mail
> 7350048 again.

[c9s, 2013-02-19 04:09:42]

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

Please take another look.

[c9s, 2013-02-19 04:15:04]

I have a question. 

the tests in the go repository are passed.

but in my external package, it seems still compile with the old xml package when
building,

I've run  ./make.bash,   ./all.bash, 

Is there something I have not updated?

how do I solve this problem?

Thanks


Cheers,
c9s




On 2013/02/19 04:09:42, c9s wrote:
> Hello
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com,
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dave@cheney.net (cc:
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com),
> 
> Please take another look.

[dave_cheney.net, 2013-02-19 04:17:18]

Without knowing the details of your system, I'd say you had two
versions of Go installed, once possibly from your os distro, the
other, trunk, from source. If this is the case, either uninstall the
distro one, or at least make sure $GOROOT/bin/go comes higher in the
path than one provided by your distro, but I strongly recommend the
first option.

On Tue, Feb 19, 2013 at 3:15 PM,  <cornelius.howl@gmail.com> wrote:
>
>
> I have a question.
>
> the tests in the go repository are passed.
>
> but in my external package, it seems still compile with the old xml
> package when building,
>
> I've run  ./make.bash,   ./all.bash,
>
> Is there something I have not updated?
>
> how do I solve this problem?
>
> Thanks
>
>
> Cheers,
> c9s
>
>
>
>
> On 2013/02/19 04:09:42, c9s wrote:
>>
>> Hello
>
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com,
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dave@cheney.net (cc:
>
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com),
>
>
>> Please take another look.
>
>
>
>
> https://codereview.appspot.com/7350048/

[c9s, 2013-02-19 06:42:23]

Weird , I have only one version Go installed (from hg), which is located at
/usr/local/go

I ran `all.bash`, `make.bash` for the changes in $GOROOT/src.

then build my external package from my GOPATH (/Users/c9s/go/src/
github.com/go-appcast)

The build succeeded, but the test failed with the same test data in Go hg.





2013/2/19 Dave Cheney <dave@cheney.net>

> Without knowing the details of your system, I'd say you had two
> versions of Go installed, once possibly from your os distro, the
> other, trunk, from source. If this is the case, either uninstall the
> distro one, or at least make sure $GOROOT/bin/go comes higher in the
> path than one provided by your distro, but I strongly recommend the
> first option.
>
> On Tue, Feb 19, 2013 at 3:15 PM,  <cornelius.howl@gmail.com> wrote:
> >
> >
> > I have a question.
> >
> > the tests in the go repository are passed.
> >
> > but in my external package, it seems still compile with the old xml
> > package when building,
> >
> > I've run  ./make.bash,   ./all.bash,
> >
> > Is there something I have not updated?
> >
> > how do I solve this problem?
> >
> > Thanks
> >
> >
> > Cheers,
> > c9s
> >
> >
> >
> >
> > On 2013/02/19 04:09:42, c9s wrote:
> >>
> >> Hello
> >
> >
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com
> ,
> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dave@cheney.net (cc:
> >
> >
>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com
> ),
> >
> >
> >> Please take another look.
> >
> >
> >
> >
> > https://codereview.appspot.com/7350048/
>



-- 
Best Regards,

Yo-An Lin

[c9s, 2013-02-19 07:00:11]

Ah, I have a symbol link from ~/src/go/go to /usr/local/go , does this
affect ?


2013/2/19 Lin Yo-An <cornelius.howl@gmail.com>

>
> Weird , I have only one version Go installed (from hg), which is located
> at /usr/local/go
>
> I ran `all.bash`, `make.bash` for the changes in $GOROOT/src.
>
> then build my external package from my GOPATH (/Users/c9s/go/src/
> github.com/go-appcast)
>
> The build succeeded, but the test failed with the same test data in Go hg.
>
>
>
>
>
> 2013/2/19 Dave Cheney <dave@cheney.net>
>
>> Without knowing the details of your system, I'd say you had two
>> versions of Go installed, once possibly from your os distro, the
>> other, trunk, from source. If this is the case, either uninstall the
>> distro one, or at least make sure $GOROOT/bin/go comes higher in the
>> path than one provided by your distro, but I strongly recommend the
>> first option.
>>
>> On Tue, Feb 19, 2013 at 3:15 PM,  <cornelius.howl@gmail.com> wrote:
>> >
>> >
>> > I have a question.
>> >
>> > the tests in the go repository are passed.
>> >
>> > but in my external package, it seems still compile with the old xml
>> > package when building,
>> >
>> > I've run  ./make.bash,   ./all.bash,
>> >
>> > Is there something I have not updated?
>> >
>> > how do I solve this problem?
>> >
>> > Thanks
>> >
>> >
>> > Cheers,
>> > c9s
>> >
>> >
>> >
>> >
>> > On 2013/02/19 04:09:42, c9s wrote:
>> >>
>> >> Hello
>> >
>> >
>>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com
>> ,
>> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dave@cheney.net (cc:
>> >
>> >
>>
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com
>> ),
>> >
>> >
>> >> Please take another look.
>> >
>> >
>> >
>> >
>> > https://codereview.appspot.com/7350048/
>>
>
>
>
> --
> Best Regards,
>
> Yo-An Lin
>



-- 
Best Regards,

Yo-An Lin

[c9s, 2013-02-19 09:51:50]

I just looked into the code, and found that getTypeInfo is actually taking 
namespace information from the struct like:

    type SparkleItem struct {
Title                   string `xml:"title"`
SparkleReleaseNotesLink string `xml:"sparkle releaseNotesLink"`
Enclosure SparkleEnclosureItem `xml:"enclosure"`
    }

and the Unmarshal does namespace translation in Decoder, which translate 
namespace like "sparkle" into its namespace url.

So in the read.go:269,  a.Name.Space is actually the namespace URL, which 
is translated, and the unmarshalling works fine for this case.


*But when marshalling this structure back to XML, it losts the namespace 
information. the "<sparkle:releaseNotesLink>" becomes 
"<releaseNotesLink>..."*

Which behavior is correct?



Thanks

corneli...@gmail.com於 2013年2月19日星期二UTC+8下午12時09分18秒寫道：
>
> Sorry I guess I had some problem with hg. 
>
> let me do that. 
>
> On 2013/02/19 04:07:49, dfc wrote: 
> > On 2013/02/19 03:44:20, c9s wrote: 
> > > Hello mailto:golang-dev@googlegroups.com (cc: 
> > mailto:golang-dev@googlegroups.com), 
> > > 
> > > Please take another look. 
>
> > Hello, 
>
> > I'm not sure how you have done this but this CL is based off a 
> revision, 
> > 38271c4ce682 that does not appear in the tree. Can you please hg sync 
> && hg mail 
> > 7350048 again. 
>
>
>
> https://codereview.appspot.com/7350048/ 
>

[c9s, 2013-02-19 09:59:09]

Please take a look at the changes,

I provided a xml test case for namespace parsing,

We got the XML like this:

<?xml version="1.0" encoding="utf-8"?>
<rss version="2.0" 
xmlns:sparkle="http://www.andymatuschak.org/xml-namespaces/sparkle"  
xmlns:dc="http://purl.org/dc/elements/1.1/">
<channel>
	<item>
		<title>Version 2.0 (2 bugs fixed; 3 new features)</title>
		<sparkle:releaseNotesLink>
			http://you.com/app/2.0.html
		</sparkle:releaseNotesLink>
		<pubDate>Wed, 09 Jan 2006 19:20:11 +0000</pubDate>
		<enclosure 
			url="http://you.com/app/Your Great App 2.0.zip"
			sparkle:version="2.0" length="1623481"
			type="application/octet-stream"
			sparkle:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh" />
	</item>
</channel>
</rss>

By using the struct:




type SparkleItem struct {
	Title                   string `xml:"title"`
	SparkleReleaseNotesLink string `xml:"sparkle releaseNotesLink"`
	Enclosure SparkleEnclosureItem `xml:"enclosure"`
}

type EnclosureItem struct {
	URL string `xml:"url,attr"`
	Length int `xml:"length,attr"`
}

type SparkleEnclosureItem struct {
	EnclosureItem
	SparkleVersion string `xml:"sparkle version,attr"`
	SparkleDSASignature string `xml:"sparkle dsaSignature,attr"`
}

type SparkleChannel struct {
	Items []SparkleItem `xml:"item"`
}

type SparkleRSS struct {
	Channel SparkleChannel `xml:"channel"`
}


We can unmarshal the XML into the object,

But when marshaling back to XML, we got this:

<SparkleRSS>
  <channel>
    <item>
      <title>Version 2.0 (2 bugs fixed; 3 new features)</title>
      <releaseNotesLink
xmlns="sparkle">&#xA;&#x9;&#x9;&#x9;http://you.com/app/2.0.html&#xA;&#x9;&#x9;</releaseNotesLink>
      <enclosure url="http://you.com/app/Your Great App 2.0.zip"
length="1623481" version="2.0"
dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh"></enclosure>
    </item>
  </channel>
</SparkleRSS>

[c9s, 2013-02-19 10:19:51]

Hi dfc,

I have a few questions about the XML namespace behavior.

1. why do we translate the namespace tag to namespace url for a.Name.Space ?
2. since we translated the namespace tags to urls, we can't compare the field
info with the token..
3. instead of replacing the .Space attribute, why don't we keep it ?

Cheers

[c9s, 2013-02-19 10:31:04]

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

Please take another look.

[c9s, 2013-02-21 07:36:11]

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

Please take another look.

[bradfitz, 2013-02-21 15:40:58]

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

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:142: 
remove this extra whitespace

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:241: 
and here

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:269: 
and here

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:275: // for attributes with namespace, we resovle
the namespace url
typo: resolve.
URL is capitalized (not a variable here)

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:276: if v, ok := p.nsResolve[ a.Name.Space ] ; ok
&& finfo.name == v + ":" + a.Name.Local {
gofmt this code.

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:469: if v, ok := p.nsResolve[ start.Name.Space ] ;
ok && finfo.name == v + ":" + start.Name.Local {
gofmt

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read_t...
File src/pkg/encoding/xml/read_test.go (right):

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:483: text , err := MarshalIndent(&rss,"","  ")
gofmt

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:487: // fmt.Printf("%s",text)
remove

https://codereview.appspot.com/7350048/diff/21001/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:488: _ = text
remove

[c9s, 2013-02-21 17:03:48]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[c9s, 2013-02-21 17:04:51]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-21 17:21:40]

First line of your commit message should be shorter.

It doesn't need to mention adding a test, because that's implicit.  You should
always add a test when fixing something.

Just make it:

encoding/xml: fix unmarshalling tags with namespace

(and then a blank line, and more information)

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

https://codereview.appspot.com/7350048/diff/32001/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:463: // for element with namespace, we need to
resovle the namespace URL
still misspelled

[c9s, 2013-02-21 17:23:15]

Hi Bradfitz, Thanks for reviewing this patch, I will fix this up soon.


On 2013/02/21 17:21:40, bradfitz wrote:
> First line of your commit message should be shorter.
> 
> It doesn't need to mention adding a test, because that's implicit.  You should
> always add a test when fixing something.
> 
> Just make it:
> 
> encoding/xml: fix unmarshalling tags with namespace
> 
> (and then a blank line, and more information)
> 
> https://codereview.appspot.com/7350048/diff/32001/src/pkg/encoding/xml/read.go
> File src/pkg/encoding/xml/read.go (right):
> 
>
https://codereview.appspot.com/7350048/diff/32001/src/pkg/encoding/xml/read.g...
> src/pkg/encoding/xml/read.go:463: // for element with namespace, we need to
> resovle the namespace URL
> still misspelled

[c9s, 2013-02-21 17:28:23]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-21 18:29:53]

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

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read.g...
src/pkg/encoding/xml/read.go:271: // for attributes with namespace, we resovle
the namespace URL
still a typo in "resolve"

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
File src/pkg/encoding/xml/read_test.go (right):

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:448: func TestUnmarshalXmlWithNamespace(t
*testing.T) {
XML, not Xml (Go style is to not mix case of acronyms)

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:449: var rss = SparkleRSS{}
var rss SparkleRSS

... is fine

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:452: t.Error(err)
t.Fatal(err)

(since you can't proceed)

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:456: t.Error("item parse failed.")
t.Fatal("no items parsed")

(the parse didn't fail.  it just didn't get what you wanted.

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:459: for _, i := range rss.Channel.Items {
don't use "i" for "item". looks like an index. use "it".

I'd do:

for i, it := range ..

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:461: t.Error("title tag parse failed.")
t.Errorf("expected a title for item %d", i)

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:464: t.Error("sparkle:releaseNotesLink parse
failed.")
t.Errorf("expected a sparkle:releaseNotesLink for item %d", i)

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:470: t.Error("can not parse attribute length")
s/can not parse/expected/, here and below.

it did parse. something.

https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:481: text, err := MarshalIndent(&rss, "", " 
")
these tests are kinda useless, since you don't look at the result of test at
all.

I would just all these MarshalIndent parts.

[c9s, 2013-02-21 18:40:17]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[c9s, 2013-02-21 18:41:11]

Thanks for your patience. I've fixed them all. :-)

On 2013/02/21 18:29:53, bradfitz wrote:
> https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read.go
> File src/pkg/encoding/xml/read.go (right):
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read.g...
> src/pkg/encoding/xml/read.go:271: // for attributes with namespace, we resovle
> the namespace URL
> still a typo in "resolve"
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> File src/pkg/encoding/xml/read_test.go (right):
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:448: func TestUnmarshalXmlWithNamespace(t
> *testing.T) {
> XML, not Xml (Go style is to not mix case of acronyms)
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:449: var rss = SparkleRSS{}
> var rss SparkleRSS
> 
> ... is fine
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:452: t.Error(err)
> t.Fatal(err)
> 
> (since you can't proceed)
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:456: t.Error("item parse failed.")
> t.Fatal("no items parsed")
> 
> (the parse didn't fail.  it just didn't get what you wanted.
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:459: for _, i := range rss.Channel.Items {
> don't use "i" for "item". looks like an index. use "it".
> 
> I'd do:
> 
> for i, it := range ..
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:461: t.Error("title tag parse failed.")
> t.Errorf("expected a title for item %d", i)
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:464: t.Error("sparkle:releaseNotesLink parse
> failed.")
> t.Errorf("expected a sparkle:releaseNotesLink for item %d", i)
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:470: t.Error("can not parse attribute
length")
> s/can not parse/expected/, here and below.
> 
> it did parse. something.
> 
>
https://codereview.appspot.com/7350048/diff/33003/src/pkg/encoding/xml/read_t...
> src/pkg/encoding/xml/read_test.go:481: text, err := MarshalIndent(&rss, "", " 
> ")
> these tests are kinda useless, since you don't look at the result of test at
> all.
> 
> I would just all these MarshalIndent parts.

[bradfitz, 2013-02-21 18:42:16]

Okay, I'm now done reviewing this for style.

Leaving for Russ or somebody who knows encoding/xml.

https://codereview.appspot.com/7350048/diff/23003/src/pkg/encoding/xml/read_t...
File src/pkg/encoding/xml/read_test.go (right):

https://codereview.appspot.com/7350048/diff/23003/src/pkg/encoding/xml/read_t...
src/pkg/encoding/xml/read_test.go:456: t.Error("no items parsed.")
t.Fatal, not t.Error, otherwise you will crash at line 468 referencing [0]

[c9s, 2013-02-21 18:44:07]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[c9s, 2013-02-23 08:45:44]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-03-12 13:28:18]

Sorry, I missed this CL and fixed the attribute problem as part of cleaning up
some other name space issues in CL 7227056. 

I looked at this to double-check what I'd done, and I don't see nsResolve
defined anywhere in the current patch set, so I don't think it compiles.

Also, the test and therefore the code are not correct. They assume that the name
space can be specified as "sparkle", but that's actually a local identifier in
the XML that is not required. That is, this XML from the test:

<rss version="2.0"
 xmlns:sparkle="http://www.andymatuschak.org/xml-namespaces/sparkle"
 xmlns:dc="http://purl.org/dc/elements/1.1/">
<channel>
	<item>
		<title>Version 2.0 (2 bugs fixed; 3 new features)</title>
		<sparkle:releaseNotesLink>http://you.com/app/2.0.html</sparkle:releaseNotesLink>
		<pubDate>Wed, 09 Jan 2006 19:20:11 +0000</pubDate>
		<enclosure 
			url="http://you.com/app/Your Great App 2.0.zip"
			sparkle:version="2.0" length="1623481"
			type="application/octet-stream"
			sparkle:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh" />
	</item>
</channel>
</rss>

is completely equivalent to this alternate form:

<rss version="2.0"
 xmlns:GOPHERS="http://www.andymatuschak.org/xml-namespaces/sparkle"
 xmlns:dc="http://purl.org/dc/elements/1.1/">
<channel>
	<item>
		<title>Version 2.0 (2 bugs fixed; 3 new features)</title>
		<GOPHERS:releaseNotesLink>http://you.com/app/2.0.html</GOPHERS:releaseNotesLink>
		<pubDate>Wed, 09 Jan 2006 19:20:11 +0000</pubDate>
		<enclosure 
			url="http://you.com/app/Your Great App 2.0.zip"
			GOPHERS:version="2.0" length="1623481"
			type="application/octet-stream"
			GOPHERS:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh" />
	</item>
</channel>
</rss>

but the CL does not accept that form because it expects "sparkle".

If I modify your test to use for example
`xml:"http://www.andymatuschak.org/xml-namespaces/sparkle releaseNotesLink"`
instead of `xml:"sparkle:releaseNotesLink"` then it does pass with my current
CL.

Let's go with CL 7227056 instead. Apologies for the duplicated effort.

[c9s, 2013-03-12 13:50:34]

Thank you for reviewing this patch.

looking forward to see this namespace issue to be resolved in go1.1. 


On 2013/03/12 13:28:18, rsc wrote:
> Sorry, I missed this CL and fixed the attribute problem as part of cleaning up
> some other name space issues in CL 7227056. 
> 
> I looked at this to double-check what I'd done, and I don't see nsResolve
> defined anywhere in the current patch set, so I don't think it compiles.
> 
> Also, the test and therefore the code are not correct. They assume that the
name
> space can be specified as "sparkle", but that's actually a local identifier in
> the XML that is not required. That is, this XML from the test:
> 
> <rss version="2.0"
>  xmlns:sparkle="http://www.andymatuschak.org/xml-namespaces/sparkle"
>  xmlns:dc="http://purl.org/dc/elements/1.1/">
> <channel>
> 	<item>
> 		<title>Version 2.0 (2 bugs fixed; 3 new features)</title>
>
		<sparkle:releaseNotesLink>http://you.com/app/2.0.html</sparkle:releaseNotesLink>
> 		<pubDate>Wed, 09 Jan 2006 19:20:11 +0000</pubDate>
> 		<enclosure 
> 			url="http://you.com/app/Your Great App 2.0.zip"
> 			sparkle:version="2.0" length="1623481"
> 			type="application/octet-stream"
> 			sparkle:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh" />
> 	</item>
> </channel>
> </rss>
> 
> is completely equivalent to this alternate form:
> 
> <rss version="2.0"
>  xmlns:GOPHERS="http://www.andymatuschak.org/xml-namespaces/sparkle"
>  xmlns:dc="http://purl.org/dc/elements/1.1/">
> <channel>
> 	<item>
> 		<title>Version 2.0 (2 bugs fixed; 3 new features)</title>
>
		<GOPHERS:releaseNotesLink>http://you.com/app/2.0.html</GOPHERS:releaseNotesLink>
> 		<pubDate>Wed, 09 Jan 2006 19:20:11 +0000</pubDate>
> 		<enclosure 
> 			url="http://you.com/app/Your Great App 2.0.zip"
> 			GOPHERS:version="2.0" length="1623481"
> 			type="application/octet-stream"
> 			GOPHERS:dsaSignature="BAFJW4B6B1K1JyW30nbkBwainOzrN6EQuAh" />
> 	</item>
> </channel>
> </rss>
> 
> but the CL does not accept that form because it expects "sparkle".
> 
> If I modify your test to use for example
> `xml:"http://www.andymatuschak.org/xml-namespaces/sparkle releaseNotesLink"`
> instead of `xml:"sparkle:releaseNotesLink"` then it does pass with my current
> CL.
> 
> Let's go with CL 7227056 instead. Apologies for the duplicated effort.

[bradfitz, 2013-06-05 22:24:00]

R=close

Resolved as part of CL 7227056