First Pass at Multiline Strings #43
Conversation
Currently they only cover the "folded" style of strings, and I'm sure there are edge cases I haven't covered yet, but it's a start.
Also fix a bug I found thanks to a mistake in a test. Still looking for more test cases to throw at it to make sure it works as expected in all scenarios
|
I went ahead and added tests and support for literal style strings; even if the implementation isn't up to snuff, the tests should still be useful |
There was a problem hiding this comment.
Looks pretty good, thanks for the excellent work!
Do takeLinesForMultilineFoldedString and takeLinesForMultilineLiteralString differ only in the takeLineForMultlineLiteralString and takeLineForMultlineFoldedString they call? If so they can be combined with a lambda parameter to reduce duplication.
Also takeMultilineLiteralString and takeMultilineFoldedString looks similar, too. If there is some reasoning not generalizing them is a better choice, please note it in the source.
Code logic looks good. And I see from the tests that they are comprehensive enough at least for now. We can't figure out what are missing until we use the test suite (#24)
|
|
||
| @BeforeTest | ||
| fun createSerializers() { | ||
| testStringDataSerializer = DecoderTest.TestStringData.serializer() |
There was a problem hiding this comment.
It's better to declare your own TestData class in this test. So DecoderTest can be changed in the future without any concern.
There was a problem hiding this comment.
Oh yeah good call, done in 531d708
My attempt at implementing multi-line strings
This is still in a fairly rough state:
>) strings are implementedHowever, if this approach is acceptable, I can add support and an equivalent set of tests for "literal" (using
|) strings as well.Please let me know if there are test cases I'm missing! Multiline strings in YAML are way more complicated than I initially realized so I'm sure there's something I haven't thought of.