Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding/xml: MarshalXML interface is not good enough #2771

Closed
mark-summerfield opened this Issue Jan 24, 2012 · 25 comments

Comments

Projects
None yet
5 participants
@mark-summerfield
Copy link

mark-summerfield commented Jan 24, 2012

Using ISO8601 is more compact and more accurate as the code below shows:

package main
import (
   "fmt"
   "os"
   "encoding/xml"
   "encoding/json"
   "time"
   )
func main() {
   type T struct { When time.Time `xml:",attr"` }
   t := &T{time.Now()}
   xml.Marshal(os.Stdout, t)
   j, _ := json.Marshal(t)
   fmt.Println(string(j))
}
// Output:

<T When="Mon Jan 16 12:29:28 +0000 GMT 2012"></T>
{"When":"2012-01-16T12:29:28.802511Z"}
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 24, 2012

Comment 1:

The obvious answer is to add a MarshalXML method to time.Time,
like we did for json and gob, but it is not clear to me that the xml
package's Marshaler interface is really thought out.  It appears to
be responsible for including the xml tags <Foo></Foo>, but that
means that it cannot be used for attributes, and worse I don't think
it has the information available to figure out what tag to use:
the field name is unavailable.
For Go 1 it is possible that we should remove the xml.Marshaler
interface to enable a rethink at a later time.
Gustavo?

Labels changed: added priority-go1, removed priority-triage.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 24, 2012

Comment 2:

The workaround, by the way, is to use strings instead of times
in your data structures and then convert them yourself.
Or to use an encoding meant for data structures, like json or gob.
@mark-summerfield

This comment has been minimized.

Copy link
Author

mark-summerfield commented Jan 24, 2012

Comment 3:

Yes, the workaround is easy & I do it; this was more FYI & to encourage a useful
consistency.
@niemeyer

This comment has been minimized.

Copy link
Contributor

niemeyer commented Jan 24, 2012

Comment 4:

Agreed, I'd prefer to take the interface off as well, and bring it back after Go 1 with
a matching unmarshaler interface (which doesn't exist in any style right now).

Owner changed to @niemeyer.

@mark-summerfield

This comment has been minimized.

Copy link
Author

mark-summerfield commented Jan 24, 2012

Comment 5:

I'll be v. sorry if it goes so near to Go 1. (But I'm biased since I use it in my Go
book. Ironically, I'd started out using the xml parser & I guess I'll have to return to
that approach.)
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 24, 2012

Comment 6:

I don't understand what you mean.  If you are using the xml parser
that is for reading XML; we are talking about changing one detail
of writing XML.
@mark-summerfield

This comment has been minimized.

Copy link
Author

mark-summerfield commented Jan 25, 2012

Comment 7:

I must have misunderstood. I thought you were talking about removing xml.Marshal
altogether (as per comments 1 and 4); but if you're just talking about improving one
small behavior of xml.Marshal that's great.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 25, 2012

Comment 8:

We removed the xml.Marshaler interface (not the xml.Marshal function),
so that we can introduce a new and improved version after Go 1.
You were only using the xml.Marshaler interface if you were implementing
your own MarshalXML methods.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 25, 2012

Comment 9:

Now that the xml.Marshaler interface is gone, this is no longer a Go 1 bug.

Labels changed: added priority-later, removed priority-go1.

@mark-summerfield

This comment has been minimized.

Copy link
Author

mark-summerfield commented Jan 26, 2012

Comment 10:

I've just switched to tip. ISTM that this has made things worse. Before I had a Thing
struct and a parallel XMLThing struct (with the xml-specific tags). Then I could write
the xml.Header and <THINGS> myself, and then convert each Thing to an XMLThing one
at a time for writing, finally writing </THINGS> myself. But now that doesn't seem
possible so I have to create an entire parallel data structure in memory. However, I
guess that once the new xml.Marshaler interface is done, I'll be able to do the
conversions on the fly.
Also, I notice that the xml.Encoder doesn't support time.Time at all now.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jan 26, 2012

Comment 11:

Hi Mark.
I think we are talking about different things.  Can you please post some code?
Russ
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jun 24, 2012

Comment 12:

Labels changed: added go1.1.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 13:

Gustavo, do you want to start having this conversation? If we're going to get this into
Go 1.1 we should be thinking about it now.
Thanks.
Russ
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 14:

Issue #4016 has been merged into this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 12, 2012

Comment 15:

Labels changed: added go1.1maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 14, 2012

Comment 16:

Labels changed: removed go1.1maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 10, 2012

Comment 17:

Labels changed: added size-l.

@kortschak

This comment has been minimized.

Copy link
Contributor

kortschak commented Feb 23, 2013

Comment 18:

I have looked around for discussion of design intentions for this issue, but have found
nothing except what is here. In the absence of this discussion, I have put together a CL
(https://golang.org/cl/7379049/) for xml.Unmarshaler support.
This is principally for my own use as a forked package while the standard library does
not yet support Unmarshaler, but I figured it may provide some basis for discussion.
The CL as it stands does not correctly handle `xml:"A>B"` tags, feeding the entire A
element to the Unmarshaler - I have not been able to figure out how to retain the B
start offset without becoming horribly tangled. Because of this I have not included
tests in the CL.
If this is not completely off track I can spend more time on handling sub-elements and
submit it for formal review (as it stands it is satisfactory for my use).
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 12, 2013

Comment 19:

I am sad to say it, but I think we will have to postpone XML work until
after Go 1.1.
I regret that we didn't have more time to make encoding/xml better, but
given the tradeoff I think focusing on core performance and
implementation pieces for this final release push is probably the right
choice. Unlike most of the performance and other stuff we're trying to
shake out right now, functionality such as XML parsing can be provided
by go get-able libraries as a stopgap until Go 1.2.
More comments specific to CL 7379049 on the CL.

Labels changed: added go1.2, removed go1.1.

Owner changed to ---.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 12, 2013

Comment 20:

[The time for maybe has passed.]
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 21:

golang.org/s/go12xml
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 22:

Labels changed: added feature.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Aug 14, 2013

Comment 23:

This issue was closed by revision 85f3acd.

Status changed to Fixed.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Aug 14, 2013

Comment 24:

This issue was closed by revision 56ce83f.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Aug 14, 2013

Comment 25:

This issue was closed by revision 54bdfc0.

@rsc rsc added this to the Go1.2 milestone Apr 14, 2015

@rsc rsc removed the go1.2 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.