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

XML Archive Tag Names #395

Closed
hoensr opened this issue Apr 11, 2017 · 4 comments
Closed

XML Archive Tag Names #395

hoensr opened this issue Apr 11, 2017 · 4 comments
Milestone

Comments

@hoensr
Copy link

hoensr commented Apr 11, 2017

Hello!

So, suppose I have this DTO:

struct Foo
{
    std::vector<int> BarList;

    template<class Archive>
    void serialize(Archive & archive)
    {
        archive(
            CEREAL_NVP(BarList)
            );
    }
};

Cereal will serialize it to XML like this:

<?xml version="1.0" encoding="utf-8"?>
<cereal>
        <BarList size="dynamic">
                <value0>18</value0>
                <value1>20</value1>
                <value2>2</value2>
                <value3>3</value3>
                <value4>4</value4>
        </BarList>
</cereal>

There are three things I don't like about this XML:

  1. The "cereal" root tag... This is solved by PR 321.
  2. I'd like to have a different inner tag name than the funny "value0", "value1", ...
  3. I don't need the size="dynamic" attribute. What was it good for, anyway?

Therefore, I have forked the repo and added some patches for these issues. Of course, the default behaviour is unchanged, but since I didn't want to add another boolean argument (don't you just love these Options(true, true, false, true, false) constructors??), I went for sort of a fluent interface.

Now with this code:

struct Foo
{
    std::vector<int> BarList;

    template<class Archive>
    void serialize(Archive & archive)
    {
        archive(
            CEREAL_NVP(BarList).withInnerName("Bar")
            );
    }
};

    {
        Foo foo{ { 18, 20, 2, 3, 4 } };
        cereal::XMLOutputArchive xmlwrite(
            std::cout, 
            cereal::XMLOutputArchive::Options(4, true, false, "MyFoo").NoSizeAttributes());
        foo.serialize(xmlwrite);
    }

My XML looks like this:

<?xml version="1.0" encoding="utf-8"?>
<MyFoo>
        <BarList>
                <Bar>18</Bar>
                <Bar>20</Bar>
                <Bar>2</Bar>
                <Bar>3</Bar>
                <Bar>4</Bar>
        </BarList>
</MyFoo>

So, my additions were:

  1. I have added the withInnerName function to the NameValuePair to control the name of the inner XML tags.
  2. I have added the NoSizeAttribute function to the XMLOutputArchive::Options class to get rid of the attribute.

My branch is currently here. Since I have not so much experience with creating pull requests and having them merged, I'd first like to ask whether this addition is interesting?

Then, since I gather there are already people thinking about a different Options structure, maybe it would be nice to have more understandable options, something like this?

Options().Precision(4).NoIndent().NoSizeAttributes()

@AzothAmmo
Copy link
Contributor

Regarding your enumerated points first:

  1. I will hopefully get around to merging this soon since 1.3 is our next big milestone.
  2. The default naming scheme is just one that supports out of order loading. I don't see anything wrong with allowing it to be changed.
  3. This actually serves no direct functional purpose internally. It exists to let a human know that cereal could potentially handle them directly manipulating the number of elements here and their program would not crash when loading. Consider an std::array instead of an std::vector, although in both cases cereal can get the number of elements from parsing the XML, for the array, bad things will happen if too many elements are read or written into the fixed sized array.

I'm definitely a fan of cleaning up the options interface. It was fine when we had only a few options but it is starting to become very unwieldy, as you demonstrated.

I'm not sure I like the interface you propose for the "inner name" of NVP values, though I do like giving this as an option. It might just be the naming I don't like, I'd need to think about it more.

If you do decide to make a PR, try to distill each major change into its own independent PR. All of these changes would also need to be mirrored in the JSON archive as well (which you don't have to do), but know that it would be part of merging any of them.

@hoensr
Copy link
Author

hoensr commented Apr 12, 2017

Hi Azoth,

thank you for your kind reply! I have created two pull requests for my two changes:

Hope this is helpful.
Of course, feel free to change the details (naming, options handling etc.) to your liking.

About JSON, I don't really see a need to touch it, because in JSON the result is already fine as it is:

{
    "BarList": [
        18,
        20,
        2,
        3,
        4
    ]
}

@AzothAmmo
Copy link
Contributor

#401 merged

@AzothAmmo AzothAmmo added this to the v1.2.3 milestone May 5, 2017
@AzothAmmo
Copy link
Contributor

Closing as #400 is self contained.

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