Writing of empty vectors (and maybe mappings) fails

[johan-gson]

Description

If you create a sequence node with zero elements and write it to a string using serialize, you end up with this:
example_vector:

when it should look like for example this:
example_vector: []

I did a fix for this that solved it for my case, but I doubt it deals with all cases, and there is probably a better way to solve it:

line 204, serializer.hpp, old code:

            const bool is_scalar = itr->is_scalar();
            if (is_scalar) {
                str += " ";
                serialize_node(*itr, cur_indent, str);
                str += "\n";
            }
            else {
                str += "\n";
                serialize_node(*itr, cur_indent + 2, str);
            }

new code:
const bool is_scalar = itr->is_scalar();
if (is_scalar) {
str += " ";
serialize_node(*itr, cur_indent, str);
str += "\n";
}
else if (itr->is_sequence() && itr->size() == 0) {
str += " []\n";
}
else {
str += "\n";
serialize_node(*itr, cur_indent + 2, str);
}

Thanks for the nice work! Loading seems to support this, so no problem there!

Reproduction steps

Just create a tree with a child node that is an empty sequence.

Expected vs. actual results

Given in the description above.

Minimal code example

No response

Error messages

No response

Compiler and operating system

Windows 10, Visual c++ 2022

Library version

latest, 43a1884

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[fktn-k]

@johan-gson
Thanks for filing the issue and suggesting a fix!
You're right. The current library doesn't properly serialize empty sequences.

Although I've confirmed that this is also the case for empty mappings, let's put aside that for now since a similar fix should be applicable.

The suggested fix should resolve your case, but it only assumes an empty sequence to be a mapping value. The YAML specification allows it to be a mapping key, sequence element or root node as well.

--- # a mapping key
? []
: foo
# `[]: foo` is the equivalent, but the library doesn't serialize sequences this way (for now).

--- # the root node
[]

--- # a sequence element
- []

Since the line 140 of serializer.hpp will be executed for each of them, I guess it's better to check emptiness before the for loops for sequence elements.

[johan-gson]

Thanks, yep, I suspect it only covers a subset. Line 140 was my first attempt :). It turns out a "\n" is added before getting there (in the else in the code I modified above), leading to that the "[]" ends up on a new line, which is not desired (I think at least). But there is probably some smarter way to solve it!

[fktn-k]

@johan-gson

leading to that the "[]" ends up on a new line, which is not desired (I think at least)

I agree. That uglifies output contents.

Thanks for your effort. Feel free to ask if you need anything!

[johan-gson]

Am I allowed to commit directly into develop - I could easily fix it?

[johan-gson]

Or if you allow me to create a branch from develop followed by a pull request.

[johan-gson]

Or if you want to do it yourself - that would of course be the best :)

[fktn-k]

@johan-gson

Am I allowed to commit directly into develop - I could easily fix it?

Follow the contribution guideline described in CONTRIBUTING.md. Basically, 1) fork this repository, 2) fix the issue there and 3) submit a pull request.

Or if you want to do it yourself - that would of course be the best :)

I was thinking you're addressing it. Of course, I can do it but it would take a few days.

[johan-gson]

Ok, I did a pull request, also fixing the empty mappings. I couldn't get the test framework to run, so I didn't do any tests - I don't have time to dig into that. I'm not super worried, but it would be good if someone could run all the tests just to make sure everything works as before (except the empty stuff, then), that would be good.

[johan-gson]

I did test it a bit with my own code, where it worked.

[fktn-k]

@johan-gson
Thanks for the PR. I just merged it to develop.
Regarding test cases, I'll take care of them later.

[johan-gson]

Thanks!

…

On Mon, Jan 6, 2025 at 3:13 AM fktn ***@***.***> wrote: @johan-gson <https://github.com/johan-gson> Thanks for the PR. I just merged it to develop. Regarding test cases, I'll take care of them later. — Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHX2AK56MQ5XT5BJUBGVAMT2JI3K3AVCNFSM6AAAAABUUBM5M2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZSGUZDKMJZGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[fktn-k]

@johan-gson
The PR #457 has added test cases for serualizing empty sequences and mappings.
While I was working on it, I noticed that the fix doesn't cover the cases where such nodes are a single root node. I'll keep this issue opened until those cases gets covered.

[johan-gson]

You're right - what should that look like? there is no name for the top node, right? Should it just look like "{}\n"? ChatGPT had many suggestions, this was one of them

[johan-gson]

and [] for a sequence

[fktn-k]

Yes, exactly.

[johan-gson]

Ok, I sent in a new pull request

[johan-gson]

Seems difficult to merge, not sure what happened, I'm hoping you can sort it out

[fktn-k]

Thanks for the PR.
Your fork repository doesn't seem updated with recent changes made in this repository. That's just why conflicts happen.

[fktn-k]

@johan-gson
Added some test cases for the fix you've made in the PR #461.
I'll close this issue since everything has been addressed for this issue.
If you find anything related but not yet resolved, feel free to reopen it.

Anyway, thanks a lot for your contributions!