feat: Make Yaml nodes serializable #642
Conversation
EdwarDDay
commented
Nov 17, 2024
EdwarDDay
commented
- fixes Make YamlNode serializable #641
EdwarDDay
changed the title
src/commonMain/kotlin/com/charleskorn/kaml/YamlNodeSerializer.kt
Outdated
Show resolved
Hide resolved
| try { | ||
| return encoder.encodeBoolean(value.toBoolean()) | ||
| } catch (_: SerializationException) { | ||
| } | ||
| try { | ||
| return encoder.encodeLong(value.toLong()) | ||
| } catch (_: SerializationException) { | ||
| } | ||
| try { | ||
| return encoder.encodeDouble(value.toDouble()) | ||
| } catch (_: SerializationException) { | ||
| } |
There was a problem hiding this comment.
Rather than this chain of try / catch, couldn't we inspect the value and determine the best option to use?
There was a problem hiding this comment.
I didn't want to copy the parsing code, to avoid duplicated code. Do you have an idea how to implement this without duplicating most of the YamlScalar code?
I mean Boolean has extra parsing code, integer values extra checks for octal, decimal and hexadecimal numbers, floating point numbers have extra checks for infinity and NaN. This was too much code for me to copy. Else the library gets inconsistent with future changes.
There was a problem hiding this comment.
What if we just always encode the value as a string?
There was a problem hiding this comment.
Then we would encode a double 1.2 as "1.2", right? So the output might be unexpected to the user.
I could create toBooleanOrNull, toByteOrNull, … functions in the YamlScalar class and use it in toBoolean, toByte, … functions and in the YamlScalarSerializer. This way the code doesn't get duplicated and we don't have to try catch everything.
- fix small issues
| value.content.singleOrNull()?.also { return encoder.encodeChar(it) } | ||
| encoder.encodeString(value.content) |
There was a problem hiding this comment.
Is there a test for this special handling of characters?
There was a problem hiding this comment.
No, I just wanted to use the short version of encodeChar, to not have the overhead from encodeString. There are also no extra tests for encodeBoolean, encodeLong, encodeDouble. I thought, these are properly tested in the tests for YamlOutput.
But I can of course write extra tests, if you think, that's needed.
| try { | ||
| return encoder.encodeBoolean(value.toBoolean()) | ||
| } catch (_: SerializationException) { | ||
| } | ||
| try { | ||
| return encoder.encodeLong(value.toLong()) | ||
| } catch (_: SerializationException) { | ||
| } | ||
| try { | ||
| return encoder.encodeDouble(value.toDouble()) | ||
| } catch (_: SerializationException) { | ||
| } |
There was a problem hiding this comment.
What if we just always encode the value as a string?
