-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Conversation
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.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
FAIL(); | ||
} catch (CanteraError& err) { | ||
EXPECT_THAT(err.what(), | ||
testing::HasSubstr("Key 'b' not found or contains no value")); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks!
Changes proposed in this pull request
bad conversion
errors while parsing.Checklist
scons build
&scons test
) and unit tests address code coverage