-
Notifications
You must be signed in to change notification settings - Fork 59
Add SingleLineStringStyle.PlainExceptAmbiguous and AmbiguousEscapeStyle #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SingleLineStringStyle.PlainExceptAmbiguous and AmbiguousEscapeStyle #371
Conversation
For the name, PlainEscapedSingle and PlainEscapedDouble could work instead and that would mean ambiguousEscapeStyle could be removed. Let me know what you think @charleskorn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @russellbanks. Overall, this looks good to me.
I've got a couple of suggestions for some more test cases, and there are some other values that are ambiguous that need to be considered as well.
src/commonMain/kotlin/com/charleskorn/kaml/YamlConfiguration.kt
Outdated
Show resolved
Hide resolved
I like the names you've used, and the separation between the two properties makes sense to me. |
@charleskorn I've added KDoc for PlainExceptAmbiguous and test cases for when the configuration is PlainExceptAmbiguous and the string is unambiguous and for non-string values. I've changed the isAmbiguous() function to use toBigIntegerOrNull() instead of toLongOrNull() as otherwise it won't consider strings that represent a value greater than the max value of a Long as ambiguous. I've also added the other patterns from YamlScalar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @russellbanks.
Two small nits and a minor change for hexadecimal and octal values, and then I think this is good to go.
src/commonMain/kotlin/com/charleskorn/kaml/YamlConfiguration.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/com/charleskorn/kaml/YamlConfiguration.kt
Outdated
Show resolved
Hide resolved
Looks like there are some linting issues as well - run |
Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>
Done @charleskorn |
Thanks @russellbanks! |
This pull request adds SingleLineStringStyle.PlainExceptAmbiguous that escapes Strings that would be a different type if they were not a String. Resolves #367.
For example, when SingleLineStringStyle is set to PlainExceptAmbiguous, "12" will become "12" in Yaml, whereas Plain would have it be 12.
I have also added an extra configuration option of ambiguousEscapeStyle so the user can choose the escape style for when the string is ambiguous. If it is set to SingleQuoted, then the above example would be escaped like '12', rather than "12". This defaults to DoubleQuoted.
The private function String.isAmbiguous() deems a String as not a String if it was not a String by parsing it as a Long, Double and Boolean. If it is none of these, then it is not ambiguous and we can leave it as plain.
I have written 6 tests that test an Int, Float, and Boolean with each escape style.
It fallbacks to Plain for multiline strings.
I don't tend to contribute to libraries so let me know if I've missed anything or there's anything that can be done better :)