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

Os serializer #45

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

oa-metaswitch
Copy link

Hi all.

I started working on a serializer a while back, and didn't realize that a different version had been submitted in the meantime. Unfortunately mine behaves a bit differently, so it would be difficult to merge it with the other serializer.

I thought I'd submit the code I have, though. It's reasonably feature complete, with support for maps, structs, tuples, and their various combinations, as well as sequences and XML namespaces.

Any chance this could be of use?

@oli-obk
Copy link
Collaborator

oli-obk commented Sep 5, 2017

phew... could you run rustfmt? It's very hard to check out with all these unrelated formatting changes.

@oa-metaswitch
Copy link
Author

Sorry about that.

My version of rustfmt seems happy with those changes. Should I be running with a particular version of rustfmt?

@oli-obk
Copy link
Collaborator

oli-obk commented Sep 5, 2017

Are you using rustfmt-nightly or rustfmt?

@oa-metaswitch
Copy link
Author

oa-metaswitch commented Sep 6, 2017

Looks like I'm using rustfmt. I'll try to run rustfmt-nightly and see how that goes. Thanks. :) [edited because I had it backwards]

@oa-metaswitch
Copy link
Author

That did the trick. Thanks. :)

@@ -115,8 +120,10 @@ fn nested_collection() {

let s = r##"
<project name="my_project">
<item name="hello1" source="world1.rs" />
<item name="hello2" source="world2.rs" />
<items>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this form of sequence processing doesn't match how a lot of xml is written. I personally would prefer it this way though. Together with serde-rs/serde#690 this would be the best solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a choice that I believe matches up better with the requirements of serde, but indeed it does not match all xml schemas.

We could make it globally configurable in the serializer and deserializer, perhaps?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely doesn't match what I needed to parse and created serde-xml-rs for in the first place unfortunately :(

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this representation is already supported with extra struct containing the collection, while adapting the other way is much harder.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Using a serde annotation actually matches up with what I had to do to wrap primitives when serializing, so there's parity there.

I'm happy to revert this change. I see two options:

  • Revert it and just use serde annotations to handle wrapped lists, as it was.
  • Also add a flag to the deserializer, indicating whether all sequences should be deserialized as wrapped. I think this would be convenient for a lot of xml schemas that do wrap lists, and should cover most cases (on the assumption that schemas are likely to either always or never wrap list elements).

Any thoughts on these two options? I prefer the second one for convenience, but it does make the deserializer a bit more clunky from a design point of view.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you don't need to do anything on this end if serde-rs/serde#690 is resolved on the serde end. In that case we'd use your serializer, fix the deserializer to match and just require the annotations in @RReverser's cases.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if serde itself gets support for aggregating duplicate fields into vectors, that would completely cover my use case.

@bjgill
Copy link

bjgill commented Jun 13, 2018

As an interested third party, is this PR likely to get merged at any point in the near future?

@oa-metaswitch
Copy link
Author

@RReverser: sorry to bother you again, but it's been a year and I'm keen that this thread doesn't go stale. It doesn't look like the serde feature is going to happen anytime soon, so would it make sense to either merge this as is or make some change so we can support both list types?

@punkstarman
Copy link
Collaborator

Looking at the PR, I noticed that xml_rs is not used to serialize, rather xmltree which requires keeping the whole DOM tree in memory before writing to a String or Write implementation.

This is clearly not scalable.

I would rather use the xml_rs writer to implement serialization. The only difficulty I've found is handling attributes.

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

Successfully merging this pull request may close these issues.

None yet

5 participants