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
proposal: encoding/xml: add ability to create decoder with start element already on stack #49313
Comments
Ping |
We haven't gotten this out of incoming yet, sorry. It won't be for Go 1.18. |
This proposal has been added to the active column of the proposals project |
I'm confused about
Why does start appear twice? What is wrong with the current API which allows
? |
This contrived example shows the error: https://go.dev/play/p/P2n9VLzEQX6 Because the start token has already been popped from the stream (to check what the name is to trigger the appropriate handler or callback) it is never read through the decoder. If you call decode there is no start element on the stack and the XML appears invalid. There is a workaround as detailed in the initial post, but this requires yet another wrapper and looks rather ugly. |
I still need to take time to understand this better. |
Instead of xml.NewTokenDecoderElement, what about adding a Push method to Decoder, like:
Then NewTokenDecoderElement = NewTokenDecoder+Push, but Push could be used in other contexts as well. Thoughts? |
That seems better to me at first glance since it's a lot more flexible. I'm assuming any XML errors would be deferred until the token was read back out? I'll go stick that in some of the code where I would have used |
Actually, looking again this isn't the same thing. The next call to |
Apologies for the delay in replying. I don't understand this comment. It sounds like you are saying that Push cannot be used to "un-read" a token from a decoder. I agree with that. But I thought we were talking about prepping a new decoder that hasn't been used at all yet. In that case, it seems like
should work. It's true that it will never make sense to push an element that was just read from the TokenDecoder. But you could imagine other manipulations that might make sense, such as peeking at the next element from r to see if it's something you want to modify and then pushing that directly. |
I'm having a hard time expressing this in an example as it either becomes very long or doesn't look like it makes sense. The question is, with your proposal, which of the following would be returned (ignoring for a moment that it's silly to pop a token just to push it immediately):
|
d.Token would return one, but that's not how you'd use d.Push. Instead you would make a new decoder, as in your original comment above, and seed it with "one". |
Right, so in an example like the following:
If we wanted to use push inside
Instead, it would be nice if we could avoid the unnecessary |
Paging this back in again. I'm OK with having d.Push and d.Token and having to use both in that example. Because you might just as often use
Of course as was noted in the initial post, a custom xml.TokenReader can do the same already, something like
I'm not sure there's really a huge amount of difference between those, and since third-party implementations of xmlcat already exist (including your xmlstream.MultiReader), I think maybe we should leave things as they are. I thought for a bit about Push and I can't decide which semantics of Push you'd really want. That is, if you want to push |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
In a project that includes handlers similar to
http.Handler
except reading from XML, not the byte stream directly, I frequently find myself doing something like the following (where "xmlstream" is a package that contains the functionality of theio
package except dealing with XML tokens instead of bytes):In this example we could change our handler types to pass in a decoder directly, but this isn't desirable for several reasons:
Instead I'd like to have an equivalent of the
Decode
/DecodeElement
pattern but for creating a new decoder, something like:This would allow us to pop from an
xml.TokenReader
and only create a decoder when the start element is encountered, then decode into a struct (consuming the end element) without error. The initial example then becomes:EDIT: I should mention that while this can be done in an external package using the workaround from the initial example, doing it directly in
encoding/xml
has the benefit of not doing namespace transformations on the start element (we can just callpushElement
directly). This reduces overhead and possibly other subtle issues due to double decoding and the way XML namespaces are handled./cc @rsc
The text was updated successfully, but these errors were encountered: