-
Notifications
You must be signed in to change notification settings - Fork 289
Kotlin: Support record & enum serialization #2753
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
base: main
Are you sure you want to change the base?
Conversation
| assert(jsonRecordWithOptionals == """{"vec":["A","B"]}""") | ||
|
|
||
| // ComplexEnum | ||
| val withClassDiscriminator = Json { classDiscriminator = "#class" } |
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.
This will embed "#class": "ComplexEnum.String" into the resulting JSON string, but some may want to customize the value of that property as well. We can provide this via the @kotlinx.serialization.SerialName annotation, but I'm not sure there is an elegant way to customize it via the configuration file. I'm thinking of something like the custom type configuration, e.g.:
[bindings.kotlin.serialization.ComplexEnum.String]
serial_name = "complex_enum_string"which will put "#class": "complex_enum_string" to the resulting string.
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.
Assuming you don't intend blocking this PR on that tweak, it might be worth opening a PR where we can discuss this, then add a short comment in the fixture itself which notes what you said and links to the PR, so people exploring this fixture also discovers this.
Re the idea for customizing, that seems good - another alternative might be a format similar to what we did for renaming - https://mozilla.github.io/uniffi-rs/next/renaming.html - which would be more like:
[bindings.kotlin.serialization.serial_names]
"ComplexEnum.String" = "complex_enum_string"
(I've no idea if that example makes actual sense in this context, but some degree of consistency makes some sense)
| added to your `$CLASSPATH` environment variable. | ||
| * Several JARs downloaded and their path added to your `$CLASSPATH` environment variable: | ||
| * [Java Native Access](https://github.com/java-native-access/jna#download) | ||
| * [KotlinX Serialization Core runtime](https://repo1.maven.org/maven2/org/jetbrains/kotlinx/kotlinx-serialization-core-jvm/1.6.3/kotlinx-serialization-core-jvm-1.6.3.jar) |
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.
The latest version of KotlinX Serialization is currently 1.9.0. Since the CI is also using KotlinX Coroutines 1.6.3, whose latest version is also 1.10.2, I chose a version released at the time, when Kotlin 2.0 had not been released yet.
Should we fix the Kotlin version and upgrade the dependencies here?
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.
That makes sense to me. Also, here I think it might be worth mentioning that the serialization libs are optional - eg, something like "Optionally, if you choose to use the generate_serializable_types option so that serializable types are generated, you will need ..."?
687cbb5 to
855ae4e
Compare
| #[serde(default)] | ||
| pub(super) rename: toml::Table, | ||
| #[serde(default)] | ||
| generate_serializable_types: bool, |
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.
Are there any better names for this?
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.
maybe generate_serializable_annotation? That seems to better reflect what actually changes here (ie, it doesn't change how we render the type itself)?
| Ok(textwrap::indent(&wrapped, &" ".repeat(spaces))) | ||
| } | ||
|
|
||
| fn serializable_type(ty: &Type, ci: &ComponentInterface) -> Result<bool, askama::Error> { |
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.
Would it be okay to make ComponentInterface::iter_types_in_item public and use it here?
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.
yeah, that would be fine and better than copying it
004fa58 to
58a27b5
Compare
mhammond
left a comment
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.
This is fantastic, thanks!
| added to your `$CLASSPATH` environment variable. | ||
| * Several JARs downloaded and their path added to your `$CLASSPATH` environment variable: | ||
| * [Java Native Access](https://github.com/java-native-access/jna#download) | ||
| * [KotlinX Serialization Core runtime](https://repo1.maven.org/maven2/org/jetbrains/kotlinx/kotlinx-serialization-core-jvm/1.6.3/kotlinx-serialization-core-jvm-1.6.3.jar) |
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.
That makes sense to me. Also, here I think it might be worth mentioning that the serialization libs are optional - eg, something like "Optionally, if you choose to use the generate_serializable_types option so that serializable types are generated, you will need ..."?
| @@ -0,0 +1,62 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
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.
I think it makes sense to rename this parent directory now? maybe something like "generated_serialization" or similar - this each test having a unique name would be ok (eg, "test_codable" for swift and "test_serialization" for kotlin.
I'm not too bothered if you don't want to do this here or would prefer it be done in a followup etc.
(The name "codable" always gets me - I don't immediately think "serialization" when I see it :)
| assert(jsonRecordWithOptionals == """{"vec":["A","B"]}""") | ||
|
|
||
| // ComplexEnum | ||
| val withClassDiscriminator = Json { classDiscriminator = "#class" } |
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.
Assuming you don't intend blocking this PR on that tweak, it might be worth opening a PR where we can discuss this, then add a short comment in the fixture itself which notes what you said and links to the PR, so people exploring this fixture also discovers this.
Re the idea for customizing, that seems good - another alternative might be a format similar to what we did for renaming - https://mozilla.github.io/uniffi-rs/next/renaming.html - which would be more like:
[bindings.kotlin.serialization.serial_names]
"ComplexEnum.String" = "complex_enum_string"
(I've no idea if that example makes actual sense in this context, but some degree of consistency makes some sense)
| #[serde(default)] | ||
| pub(super) rename: toml::Table, | ||
| #[serde(default)] | ||
| generate_serializable_types: bool, |
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.
maybe generate_serializable_annotation? That seems to better reflect what actually changes here (ie, it doesn't change how we render the type itself)?
| Ok(textwrap::indent(&wrapped, &" ".repeat(spaces))) | ||
| } | ||
|
|
||
| fn serializable_type(ty: &Type, ci: &ComponentInterface) -> Result<bool, askama::Error> { |
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.
yeah, that would be fine and better than copying it
58a27b5 to
cc28ea8
Compare
cc28ea8 to
c43dbde
Compare
Fixes #2751.
To-Dos:
fixtures/swift-codabletofixtures/serialization?Dockerfile-build.checksum?