-
Notifications
You must be signed in to change notification settings - Fork 280
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
Implement support for KotlinX Serialization #196
base: main
Are you sure you want to change the base?
Conversation
610b0bf
to
49e30fb
Compare
49e30fb
to
073c121
Compare
Fixes and closes apple#191 Signed-off-by: Sam Gammon <[email protected]>
073c121
to
aea8568
Compare
Adds a setting to the Kotlin code generator which controls the target Kotlin package for codegen. Applied as a prefix to the normal generated package name. The current versions of the `kotlinx.serialization` and `kotlinx.html` libraries are declared dynamically, which causes a failure when refreshing dependencies. To avoid Kotlin metadata build issues, these have been pinned. - feat(codegen): use `kotlinPackage` as prefix for kotlin codegen - feat(gradle): `kotlinPackage` property in gradle plugin - test(codegen): add tests for custom kotlin package - test(gradle): add tests for generating with custom kotlin package Signed-off-by: Sam Gammon <[email protected]>
This change adds support for a new code-gen argument, `implementKSerializable`, which results in the annotation `kotlinx.serialization.Serializable` being added to `data` classes during codegen. Relates to discussion apple#185 - feat(codegen): add support for kotlin `Serializable` annotation - feat(gradle): add `implementKSerializable` argument - test(codegen): add test for KotlinX serialization support - test(codegen): add test for both Java and KotlinX serialization - test(gradle): add test for compiling with KotlinX serialization Signed-off-by: Sam Gammon <[email protected]>
aea8568
to
a1613a1
Compare
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.
Can you rebase your changes on top of main? It's a little hard to review at the moment.
One general comment here is that we shouldn't need to actually depend on kotlinx.serialization; we can reference the class by string.
Also another note: Our codegen includes types that aren't Serializable
. This includes org.pkl.core.DataSize
, org.pkl.core.Duration
, and other subclasses of org.pkl.core.Value
.
Maybe those properties should be marked @Contextual
, if using kotlinx serialization? And we would leave it to end users to decide how these should be serialized.
val implementSerializable: Boolean = false, | ||
|
||
/** Whether to annotate data classes with [kotlinx.serialization.Serializable] */ | ||
val implementKSerializable: Boolean = false, |
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.
Here, and everywhere else:
val implementKSerializable: Boolean = false, | |
val generateKotlinSerializable: Boolean = false, |
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.
okm..
@@ -933,6 +1170,7 @@ class KotlinCodeGeneratorTest { | |||
fun `user defined type aliases`() { | |||
val kotlinCode = | |||
generateKotlinCode( | |||
// language=text |
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.
// language=text |
return this | ||
} | ||
|
||
val spec = AnnotationSpec.builder(Serializable::class).build() |
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.
We can avoid adding a dependency on kotlinx serialization by referencing kotlinx serialization by string:
val spec = AnnotationSpec.builder(Serializable::class).build() | |
val spec = AnnotationSpec | |
.builder(ClassName("kotlinx.serialization", "Serializable")) | |
.build() |
Hey @bioball, sorry I missed your review here! I'll take a look. The problem with sealed classes is a bigger issue that may need some design attention before this feature ships. |
This change adds support for a new code-gen argument,
implementKSerializable
, which results in the annotationkotlinx.serialization.Serializable
being added todata
classes during codegen.New discussion opened here
Approach
A new argument has been added to the Kotlin code generator and Gradle plugin, at
implementKSerializable
. When set totrue
, the code generator annotates data classes with@kotlinx.serialization.Serializable
.If both JVM serialization and KotlinX serialization are enabled, the code generator properly qualifies with
java.io.Serializable
andimport kotlinx.serialization.Serializable
. The two are optional and usable independent of each other.For the moment, this is filed on top of these other PRs to avoid conflicts, since they all touch the code generator; these can be rebased away, though:
implementSerializable
flag #192Transitive dependency needs
Users of this code will need to include the KotlinX Serialization compiler plugin in their build, with:
And a dependency on KotlinX Serialization Core, for the annotation, at version
1.5.0
or greater:I stopped short of adding an
api
dependency to Pkl's Kotlin runtime library to avoid adding it to consumer dependency graphs downstream. This could easily be added, though, if preferable. Gradle will generally select the newest available version in a user's build, but builds that don't include KotlinX Serialization already will have to download it if it is added this way, of course.IDEA and Kotlin warn appropriately if the compiler is not included.
Changelog
Serializable
annotationimplementKSerializable
argument