Fix `YAMLDecoder`'s handling when decoding empty strings into optional types
Background
I was checking the feasibility of using YAMLDecoder to decode a simple JSON file instead of having to switch between JSONDecoder, but I noticed a discrepancy in a really simple example:
let json = """
{
"access": ""
}
"""
struct Container: Decodable {
let access: String?
}
let decoded = try YAMLDecoder().decode(Container.self, from: json)
XCTAssertEqual(decoded.access, "") // failed: ("nil") is not equal to ("Optional("")")
This was odd, because the problem does not surface when using Yams.load(yaml:):
let json = """
{
"access": ""
}
"""
let decoded = try XCTUnwrap(try Yams.load(yaml: json) as? NSDictionary)
XCTAssertEqual(decoded, ["access": ""]) // success
It was then where I discovered #301.
Description
Disclaimer: This is my first time ever looking at this codebase, so I likely will have missed something š
Following the hints from @JensAyton, I could see in NSNull.construct(from:) that we weren't factoring in the style of the input scalar, which meant that technically this method would treat 'null' or '' as null. This however never seemed to surface as an issue when using Yams.load(yaml:) since the calling methods would perform additional checks on things like the node tag. _Decoder.decodeNil() on the other hand does not.
While I could have implemented a fix in _Decoder.decodeNil(), I felt that it might be more appropriate to put it in NSNull.construct(from:), which I did here.
In addition to the fix, I added extra test coverage to reproduce the issue and I also had to make one small adjustment to NodeTests due to this change.. I'll add a comment inline.
Finally, I also want to reiterate another comment from @JensAyton:
However, Iām leery of making a PR with this change since it may impact existing code with unfortunate dependencies on the current behaviour.
I feel the same way. I'd very much appreciate thorough eyes on this. I've added what I think is appropriate test coverage, but if you think I need to do more, or if there is a better approach to fix this, please let me know š
I checked out the CI failures and corrected some SwiftLint violations that it was erroring on, but I'm not too sure why the rest of the CI jobs failed. If you have any suggestions on what I can do to debug/replicate this locally that will help. Thanks
@jpsim I've just rebased this PR with the latest changes from main and I was just wondering if you could please trigger the CI checks again? š