8000 Add SingleLineStringStyle.PlainExceptAmbiguous and AmbiguousEscapeStyle by russellbanks · Pull Request #371 · charleskorn/kaml · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 9 commits into from
Jan 28, 2023
Merged

Add SingleLineStringStyle.PlainExceptAmbiguous and AmbiguousEscapeStyle #371

merged 9 commits into from
Jan 28, 2023

Conversation

russellbanks
Copy link
Contributor
@russellbanks russellbanks commented Jan 16, 2023

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 :)

@russellbanks
Copy link
Contributor Author

For the name, PlainEscapedSingle and PlainEscapedDouble could work instead and that would mean ambiguousEscapeStyle could be removed. Let me know what you think @charleskorn

Copy link
Owner
@charleskorn charleskorn left a 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.

@charleskorn
Copy link
Owner

For the name, PlainEscapedSingle and PlainEscapedDouble could work instead and that would mean ambiguousEscapeStyle could be removed. Let me know what you think @charleskorn

I like the names you've used, and the separation between the two properties makes sense to me.

@russellbanks
Copy link
Contributor Author

@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.

Copy link
Owner
@charleskorn charleskorn left a 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.

@charleskorn
Copy link
Owner

Looks like there are some linting issues as well - run ./gradlew spotlessApply to fix them.

@russellbanks
Copy link
Contributor Author

Done @charleskorn

@charleskorn
Copy link
Owner

Thanks @russellbanks!

@charleskorn charleskorn merged commit f4e94d1 into charleskorn:main Jan 28, 2023
@russellbanks russellbanks deleted the add-plain-except-ambiguous branch January 29, 2023 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String type loss when encoding with SingleLineStringStyle.Plain
2 participants
0