User controllable multiline string serialization for YAML#657
User controllable multiline string serialization for YAML#657liuzicheng1987 merged 6 commits intogetml:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiline YAML literals by adding a Flags enum to the Writer class and updating the serialization logic to optionally use YAML::Literal. It also includes a change to the Reader to trim trailing newlines from strings to mitigate issues with yaml-cpp's handling of multiline blocks. Feedback focuses on the potentially destructive and inconsistent nature of the unconditional string trimming in the Reader, as well as code duplication in the Writer's value insertion methods that should be refactored into a helper function.
| // This is only done for literal blocks which doesn't have tags or anchors | ||
| if (_var.node_.Tag() == "!" && yaml_str[_var.node_.Mark().pos] == '|') { |
There was a problem hiding this comment.
Does reflect-cpp aspire to support yaml-anchors and/or tags? I could handle those here with an extra private function if they're needed, I just didn't want to bloat this PR further.
|
The check failures seem to be an issue with VCPKG or how CMake cannot find ninja. Can this be caused by my edits in this PR? The only project structural change I had is the addition of a new test cpp file. |
|
@microdee, sorry it took me a couple of days to get to this. Unfortunately, the Github Actions pipeline is a bit unstable lately, it has nothing to do with your PR. Your PR looks great, I will merge it. Thank you for your contribution. |
This PR introduces
|multiline literal blocks, if they contain new-line characters.|multiline literal blocks.Writing multiline blocks
By default this feature is not enabled to preserve previous behaviour, and because it costs a call to
std::basic_string::findon all string fields.In order to enable the feature the user needs to call
rfl::yaml::writewith respective flag:Which produces
in contrast to what it would produce without the flag:
I've added another flag
string_all_literalwhich forces all string values to be a multiline literal whether it actually has new-lines or not. This may be less commonly useful, but this will spare a call tostd::basic_string::find, in case the user may have mostly long strings in their fields. When using that with the same example the output looks like this:Reading multiline blocks
While testing the latter case I've noticed that this style of multiline-string in YAML leaves a potentially unintentional new line at the end, which breaks re-serialization consistency checks. For this reason
rfl::yaml::Reader::to_basic_typenow trims the trailing new-lines from|literal block scalars. If given scalar is not a|literal block, then that's not affected.For this trick
rfl::yaml::Readerand in turnrfl::yaml::readnow needs to know about the entire YAML input, so the reader can work with offset data provided byYAML::Node::Mark(). This may be a breaking change for the user, if they were directly working withYAML::Nodeinputs. Otherwise we wouldn't know if we could safely trim trailing new-lines, as yaml-cpp doesn't expose this information directly on the node.To illustrate:
Misc
It would be interesting to borrow the wisdom of the library author and/or the community in the following topics:
I've run all YAML tests, introduced a new one for this feature, and just for safety I've run all JSON tests as well, however I didn't modify anything which would affect other serialization methods or reflections. All tests passed on my side.
Thanks for consideration and I'll be happy to address your feedbacks.