Added Configuration Support for Ignoring Empty Document #286
Conversation
There was a problem hiding this comment.
Thanks for the PR @DasLixou!
Two things:
- I have a minor name change suggestion (see comments).
- Could you please add a test for this?
src/commonMain/kotlin/com/charleskorn/kaml/YamlConfiguration.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
|
Ok I’ll add a test later :) |
|
@charleskorn What have i done wrong?
|
I believe that's just a bug in IntelliJ - does it compile OK from the command line? Try |
|
That should do it :D |
There was a problem hiding this comment.
Just a few suggestions on the tests to verify this behaves as expected and then this should be good to go!
Edit: it also looks like one of the tests is failing: https://github.com/charleskorn/kaml/runs/6479824327?check_suite_focus=true#step:8:66
| } | ||
|
|
||
| it("expect ignoring empty document because of configuration") { | ||
| shouldNotThrowAny { Yaml(configuration = YamlConfiguration(allowReadingEmptyDocument = true)).decodeFromStream<String>(bytes) } |
There was a problem hiding this comment.
In this scenario (reading a non-nullable string), what would you expect to be returned from decodeFromStream? Can we make an assertion that shows the expected behaviour?
| @@ -42,5 +44,18 @@ class JvmYamlReadingTest : DescribeSpec({ | |||
| result shouldBe 123 | |||
| } | |||
| } | |||
|
|
|||
| describe("reading an empty yaml without throwing an error") { | |||
There was a problem hiding this comment.
These changes aren't specific to the JVM, but this file is for tests that only apply to the JVM. Could you please move them to the commonTest source set?
There was a problem hiding this comment.
I tried it but the problem is that the common Kotlin version of YAML only has the default variable. If there were an expected function decodeFromStream or so it would work..
There was a problem hiding this comment.
The common Kotlin version has a decodeFromString method that could be used in the tests.
| val bytes = ByteArrayInputStream(input.toByteArray(Charsets.UTF_8)) | ||
|
|
||
| it("expect throwing an error because it's an empty document") { | ||
| shouldThrowExactly<EmptyYamlDocumentException> { Yaml.default.decodeFromStream<String>(bytes) } |
There was a problem hiding this comment.
It would be good to add tests to cover a couple of other scenarios as well. For example, reading lists, maps and Kotlin classes.
|
I dont know how, but my last braincell made it working :D - After looking at it a second time it just totally make sense. |
|
PROBLEM: When the document is empty, i cant get an object out of YamlInput. Is there any way to get the default object of an Deserialization? @charleskorn |
This probably requires changes in kotlinx.serialization. What does the built-in JSON format do if you try to deserialize an empty JSON stream to an object like this? |
|
Screw it. If anyone has more braincells, just go ahead and fork DasLixou/kaml if you want. I'll just check if the file is empty before running Kaml. This system is little more complicated. |
|
Given the issues discussed above, I'm going to close this as "won't fix". |
This adds
ignoreEmptyDocumentto the Configuration (default false)When true, it doesnt throw an
EmptyYamlDocumentExceptionwhen the document is empty.Solves #284 (from Discussion #281)