Support some new features from the updated spec #25
Conversation
Handle accessing section data at the top of the context via {{.}} and
add special-case handling for `null` sections. With these changes, all
test cases from the spec pass, except for optional new features that
are not yet implemented.
The specs submodule was updated in this PR only to verify this in CI as
well. It can be restored to point back to the fork prior to merge.
|
It appears GitHub removed support for Bionic runners: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources |
|
Alternatively the UTF-8 test could be converted into a unit/regression test, removing the need to maintain a forked spec - what do you think? |
| @@ -86,7 +86,7 @@ int Data::isEmpty() | |||
| ret = 1; | |||
| break; | |||
| case Data::TypeString: | |||
| if( val == NULL || val->length() <= 0 ) { | |||
| if( val == NULL || val->length() <= 0 || *val == "null" ) { | |||
There was a problem hiding this comment.
I think this may be better handled in the specification parsing code, as I don't think it's optimal to generally treat a string containing "null" as empty. Unfortunately, libyaml, being a c, not a c++, library, is fairly low level and doesn't handle any type conversions automatically. See e.g. #18.
This (probably) isn't an issue with the PHP extension as it has it's own conversion code using native PHP types.
I've opened another PR in #26 with this change applied but let me know if you disagree or if it breaks your use-case or w/e.
There was a problem hiding this comment.
Makes sense, I think I just misinterpreted the spec there and didn't realize that was only for literal nulls.
|
Looks good, thanks! |
Handle accessing section data at the top of the context via {{.}} and add special-case handling for
nullsections. With these changes, all test cases from the spec pass, except for optional new features that are not yet implemented.The specs submodule was updated in this PR only to verify this in CI as well. It can be restored to point back to the fork prior to merge.