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

Potentially incorrect empty line detection in Parse::_filter_block_scalar #337

Closed
NaN-git opened this issue Nov 30, 2022 · 5 comments · Fixed by #338
Closed

Potentially incorrect empty line detection in Parse::_filter_block_scalar #337

NaN-git opened this issue Nov 30, 2022 · 5 comments · Fixed by #338

Comments

@NaN-git
Copy link

NaN-git commented Nov 30, 2022

In this method if chomp != CHOMP_KEEP is true, then s.trim(" \n\r\t") is executed.
Obviously this statement shall filter empty lines, but if I read the YAML 1.2 standard correctly, then an empty line in block scalars cannot contain any \t-character.

That seems to be the reason, why rapidyaml fails this test case.

@biojppm
Copy link
Owner

biojppm commented Nov 30, 2022

Thanks for reporting. Indeed, the Y79Y case has been bothering me for some time, and that seems to be the culprit. I've been hard-pressed to find the time to look at this, and many other things.

A PR fixing this would be greatly appreciated. If you do look at this, to ensure it is covered in the unit tests, don't forget to remove that test case from the list of allowed failures.

@biojppm
Copy link
Owner

biojppm commented Nov 30, 2022

Quick note: generally, even if your theory is correct, it may (or not) be the case that the instruction you point out is causing the problem should still be executed in some other scenarios.

If that is so, you may enter an annoying game of whack-a-mole to address the different problems. Build with -DRYML_BUILD_TESTS=ON and just run the target ryml-test-run to ensure every test case runs.

@NaN-git
Copy link
Author

NaN-git commented Dec 1, 2022

Before doing a PR I wanted to bring this up for discussion because I'm not sure whether my interpretation of the standard is correct.

I executed the tests and only block literal with empty docval 6/7 fail, which are similar to the Y79Y test case, but the expected output is the empty string. Could these test cases be wrong?

@biojppm
Copy link
Owner

biojppm commented Dec 1, 2022

To be honest I don't know whether it is correct or otherwise; I cannot look closely as I am really taken up finishing a project. At a first glance, I'd say it is.

FWIW, at this point the unit tests are now extensive enough that I'd say if the changeset passes everything it should probably be correct.

And to address your question, it may very well be that in some of those corner cases, the tests I have written myself may be wrong in what they demand, and to be clear whitespace is a very frequent offender.

It has been frequent that I need to adjust those tests of mine when I fix ryml to conform to more cases of the official test suite. So just go ahead and fix those. They are convolved because they go through a multi-approach sequence of emit-parse-emit-parse, so feel free to ask any questions that may surface.

@biojppm
Copy link
Owner

biojppm commented Dec 1, 2022

BTW, for this type of situation, the YAML playground is incredibly useful

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 a pull request may close this issue.

2 participants