-
Notifications
You must be signed in to change notification settings - Fork 50
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: add YamlContentPolymorphicSerializer
, similar to JsonContentPolymorphicSerializer
#607
feat: add YamlContentPolymorphicSerializer
, similar to JsonContentPolymorphicSerializer
#607
Conversation
Optional improvement: Make the If this should be implemented in this PR, then I can do it |
Could you please elaborate on these? Perhaps they could be separate PRs to help keep this one smaller and easier to review.
Sounds like something for a separate PR. |
Previously the
Previously it was not possible to implement a content based polymorphic serializer in the case that one of the types involved is a scalar. Different from the JSON serialization library it was therefore impossible to implement something like: myCats:
myFirstCat: "Bella"
mySecondCat:
name: "Daisy"
age: 2 typealias MyCats = Map<String, CatDetails>
@Serializable(with = MyContentBasedSerializer::class)
sealed interface CatDetails {
@Serializable
value class InlineName(val name: String) : CatDetails
@Serializable
data class More(
val name: String,
val age: String?
) : CatDetails
}
object MyContentBasedSerializer : YamlContentPolymorphicSerializer<CatDetails>(CatDetails::class) {
override fun selectDeserializer(node: YamlNode) = when (node) {
is YamlScalar -> CatDetails.InlineName.serializer()
is YamlMap -> CatDetails.More.serializer()
else -> error("Unsupported node type ${node::class.simpleName}")
}
}
Will do it sometime later in a separate PR. |
Could you please split the two bug fixes out as their own separate PR? This is quite a big PR and so breaking it down into smaller pieces will make it easier for me to review. |
baaf876
to
133edeb
Compare
This PR now depends on: |
The necessary PRs are now merged so I merged main for easier review @charleskorn |
YamlContentPolymorphicSerializer
, similar to JsonContentPolymorphicSerializer
@@ -115,6 +128,8 @@ public sealed class YamlInput( | |||
private fun YamlMap.withoutKey(key: String): YamlMap { | |||
return this.copy(entries = entries.filterKeys { it.content != key }) | |||
} | |||
|
|||
private val SerialDescriptor.isContentBasedPolymorphic get() = serialName.startsWith(YamlContentPolymorphicSerializer::class.simpleName!!) |
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.
Rather than checking the name like this, what if we introduced an annotation that is added to the SerialDescriptor
created by YamlContentPolymorphicSerializer
, then checked for its presence 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.
The annotation is implemented as internal
for now. Or should it be 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.
internal
seems fine to me, it's an implementation detail.
src/commonTest/kotlin/com/charleskorn/kaml/YamlContentPolymorphicSerializer.kt
Outdated
Show resolved
Hide resolved
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 @Jojo4GH!
🎉 This PR is included in version 0.64.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
YamlContentPolymorphicSerializer
in the same manner asJsonContentPolymorphicSerializer
YamlContentPolymorphicSerializer
Decoder
does not represent the current node correctly #617Example usage: