Skip to content
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

feat(JVM): Annotations to set the ScalarStyle on a single field. #596

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

nielsbasjes
Copy link
Contributor

A first draft of what I asked for in #594
The main problem I faced is that the fun encodeString(value: String) as defined by Kotlin does not pass the descriptor and index.

So this first attempt works (on my machine) but does make that this will not work in a multithreaded situation and multiple yamls are written at the same time by the same instance.

So either that problem can be fixed in a different way or it is essentially impossible to build.

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, the general approach looks good to me.

Could you please also add a comment to the relevant properties on YamlConfiguration to explain that these annotations take precedence over any configuration there?

@nielsbasjes
Copy link
Contributor Author

I have updated the documentation a bit more.
Now in addition to the YamlConfiguration being shown as this:
image

You now also get this:

image

@nielsbasjes nielsbasjes marked this pull request as ready for review August 4, 2024 11:31
@nielsbasjes
Copy link
Contributor Author

I have handled your review comments and it now has 2 annotations YamlWriteSingleLineStringUsingScalarStyle and YamlWriteMultiLineStringUsingScalarStyle.

I have also added a few more tests: different combinations of these 2 and having a single or multiline value)

I did not (yet) add a similar annotation for the ambiguous case because I did not yet understand how that works and should work.

@nielsbasjes
Copy link
Contributor Author

NOTE: There is no documentation in YamlConfiguration for yamlNamingStrategy.
If you have a line I can add it right away.

@nielsbasjes
Copy link
Contributor Author

I've handled the feedback as best as I understand what you want to see.
Looking forward to any other changes you would like me to make.

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!

@nielsbasjes
Copy link
Contributor Author

Squashed all commits I made to make it a lot cleaner

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.

This looks good to me. Could you please fix the linting failures?

@charleskorn charleskorn merged commit 8a08ecb into charleskorn:main Sep 13, 2024
5 checks passed
@charleskorn
Copy link
Owner

Thanks @nielsbasjes!

@nielsbasjes nielsbasjes deleted the ScalarStyleAnnotations branch September 13, 2024 08:16
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.

3 participants