#292 Better handling for flow indicators in permitted scalar contexts

[stephenwhittle]

This PR addresses #292 by checking if the lexer is in a flow context before treating flow indicators, commas, or colons as terminating plain unquoted strings.

Pull Request Checklist

Read the CONTRIBUTING.md file for detailed information.

• Changes are described in the pull request or in a referenced issue.
• The test suite compiles and runs without any error.
• The code coverage on your branch is 100%.
• The documentation is updated if you added/changed a feature.

Please don't

• The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
• Please refrain from proposing changes that would break YAML specifications. If you propose a conformant extension of YAML to be supported by the library, please motivate this extension.
• Please do not open pull requests that address multiple issues.

[coveralls]

coverage: 100.0%. remained the same
when pulling 3cc8101 on stephenwhittle:bugfix-allow_flow_indicators_in_plain_scalars
into 04e1a7b on fktn-k:develop.

[stephenwhittle]

As you can see, there's a fair bit of noise on this PR from me tweaking the tests to ensure proper coverage, and forgetting to run the amalgamation script a time or two. Serves me right for working late ;)
However, is that generating a bunch of noise on your end for the failed runs?
If so, is there a way I can import your Actions workflows into my fork? Would rather do all this iteration over there prior to opening a PR, instead of spamming you with notifications about failures.

[fktn-k]

@stephenwhittle
First of all, no worries!
It didn't bother me since I've turned off the notification before when I did the same LOL.

Rather, I'm sorry for not mentioning the way of checking test coverage in the CONTRIBUTING.md,
I usually develop this library and run lcov & genhtml (as done in the Makefile here) on Ubuntu 22.04LTS (WSL2).
I use the default version fetched from the apt repository.
If you don't use WSL, though not for the "pure" Windows environment, I've seen some article which said that the tools are also available with pacman on MSYS2.
Hope this will help.

is there a way I can import your Actions workflows into my fork?

Regarding this one, I honestly don't know at all...
I'll look for some ways to achieve that for future contributions though.

[fktn-k]

@stephenwhittle
I've found a way in which you can run the actions in your fork repository after some experiments in my fork from another one.

1. Go to the "Settings >> Actions >> General" settings section
2. Make sure the "Actions permissions" setting in your fork repository is not set to "Disable actions".
3. Update the develop branch in your fork repository up-to-date
4. Create a PR to YOUR develop branch to check the workflow results.
   When your changes get ready, just discard the PR if it's not necessary anymore.
5. Create a PR to my develop branch as usual.

I've confirmed that the above steps do nothing to the upstream repository.
If they wouldn't work to this repository... I don't care! Just give it a try!

[stephenwhittle]

A few notes @fktn-k -

• Coveralls gives me a clickable link to the build, but when trying to inspect a coverage report for a specific file I get HTTP 500 errors from their site. It would be worth considering an improvement to the code coverage Github action, so that the coverage data for a failed check is dumped into the action's log or otherwise made available for the contributor to access in some way. I eventually got coverage information generated using WSL2 as per your suggestion, but for a one-off contributor an alternative way to view a failed report would be a usability improvement.

• The coverage issue was caused by test_next_char checking the current character to detect the end of input, which of course will never be true in the context of the code in which I was using it. The end-of-input check is being tested elsewhere in the codebase, but I suspect test_next_char was being inlined at my call-site, causing the coverage tests to want me to explicitly test the end-of-input check myself.

  • It might be worth refactoring or restructuring such a function so it can be used more generally, as it's less prone to errors in the case of get_next (ie no risk of a forgotten call to unget) when we only need a single-character lookahead.
  • It may also be worth investigating if there are other compiler flags we should be passing to disable inlining during code coverage checks as it seems to be occurring even at O0 optimization level.

[fktn-k]

@stephenwhittle
Thanks for sharing the notes!

Coveralls gives me a clickable link to the build, but when trying to inspect a coverage report for a specific file I get HTTP 500 errors from their site.

This happens to me as well... (I got a 504 (timeout) error. The same on your end?)

The same error happens to other repositories (see this issue), and there seems no way to get a report back from Coveralls (see this comment). So, I'm looking to create a CI step to upload a zipped HTML view generated from the uploaded coverage data, what do you think?

It might be worth refactoring or restructuring such a function so it can be used more generally, as it's less prone to errors in the case of get_next (ie no risk of a forgotten call to unget) when we only need a single-character lookahead.

That's more reasonable. I agree with the change to make it less prone to errors, like replacing test_next_char with something like peek_next which just returns a next character without changing the current position.

It may also be worth investigating if there are other compiler flags we should be passing to disable inlining during code coverage checks as it seems to be occurring even at O0 optimization level.

Agree. I made some workarounds to avoid such issues before (I just recalled that). There should be some option to keep the compiler from expanding any functions inline.

[fktn-k]

Looks good to me. Thanks a lot, @stephenwhittle.