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

[Input] Improve handling of YAML null values #897

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

speth
Copy link
Member

@speth speth commented Jul 9, 2020

Changes proposed in this pull request

  • YAML documents containing null values no longer generate bad conversion errors while parsing.
  • Modify error messages generated by null values to indicate that the error can be caused by either a missing key or a key with no value.

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

YAML documents containing null values no longer generate errors while parsing.

Modify error messages generated by null values to indicate that the error can
be caused by either a missing key or a key with no value.
@speth speth added the Input Input parsing and conversion (for example, ck2yaml) label Jul 9, 2020
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #897 into main will increase coverage by 0.17%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
+ Coverage   70.95%   71.12%   +0.17%     
==========================================
  Files         376      376              
  Lines       45889    46212     +323     
==========================================
+ Hits        32560    32869     +309     
- Misses      13329    13343      +14     
Impacted Files Coverage Δ
include/cantera/base/AnyMap.inl.h 83.17% <50.00%> (ø)
test/general/test_containers.cpp 99.57% <87.50%> (-0.43%) ⬇️
src/base/AnyMap.cpp 83.99% <100.00%> (+0.21%) ⬆️
src/thermo/Phase.cpp 82.63% <0.00%> (-1.27%) ⬇️
samples/cxx/flamespeed/flamespeed.cpp 94.59% <0.00%> (-0.24%) ⬇️
include/cantera/thermo/Phase.h 86.04% <0.00%> (ø)
test/thermo/ThermoPhase_Test.cpp 100.00% <0.00%> (ø)
include/cantera/thermo/ThermoPhase.h 28.28% <0.00%> (ø)
src/thermo/ThermoPhase.cpp 76.43% <0.00%> (+6.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 047e74b...1d2802b. Read the comment docs.

FAIL();
} catch (CanteraError& err) {
EXPECT_THAT(err.what(),
testing::HasSubstr("Key 'b' not found or contains no value"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the tests pass, but I'm surprised this isn't raised for key 'c'. Is that a limitation of yaml-cpp?

Copy link
Member Author

@speth speth Jul 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yaml-cpp correctly converts both the blank value and ~ to null. The difference here is that the exception is raised by asString(), not at("b"). I guess in this sense, the AnyMap class isn't quite a faithful representation of YAML in that in AnyMap, a key with a null value is equivalent to the key not being present, while YAML makes a distinction. Since we don't use null values anywhere in the Cantera YAML format, I'm not sure whether it's worth the extra effort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

FAIL();
} catch (CanteraError& err) {
EXPECT_THAT(err.what(),
testing::HasSubstr("Key 'b' not found or contains no value"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

@bryanwweber bryanwweber merged commit cf1c081 into Cantera:main Jul 15, 2020
@speth speth deleted the fix-yaml-null branch November 20, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Input Input parsing and conversion (for example, ck2yaml)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants