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

Unify reader::XmlEvent and writer::XmlEvent #144

Open
whitequark opened this issue Apr 23, 2017 · 2 comments
Open

Unify reader::XmlEvent and writer::XmlEvent #144

whitequark opened this issue Apr 23, 2017 · 2 comments

Comments

@whitequark
Copy link

I think it's possible to eliminate the Name/OwnedName, Attribute/OwnedAttribute, reader::XmlEvent/writer::XmlEvent, etc, dualities by switching their fields to use Cow everywhere, and then replacing reader::XmlEvent with writer::XmlEvent (and adding a few more Cows).

It also seems (to me) that the result would be more elegant, and easier to work with in downstream code, but I understand that I may be missing something.

@whitequark
Copy link
Author

Also, I've just discovered that, apparently, a new copy of Namespace is passed into each reader::XmlEvent::StartElement and that seems very bothersome to me, and easily solvable with Cow.

@netvl
Copy link
Owner

netvl commented Apr 24, 2017

I thought about doing so initially, but I'm not really sure if it actually will be convenient. Also, it probably wasn't possible way back when I started writing the library. Maybe something has changed, and it is possible and convenient to do now; this probably should be investigated.

As for your second observation, I'm afraid it is not that simple. If you do not want to pass a separate instance of Namespace into each event, then you need to pass a borrow of one. Where the owned version of the namespace should be stored then? It cannot be stored in the reader and shared between multiple events, because each event may potentially have different namespace configurations. The only way to make it consistent is to put a new instance of Namespaces into each event, just like with names and other properties.

If you want, you may try to change the API to be Cow-based and send a PR. If it works out and provides more convenience than the separate event enums, I would certainly be willing to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants