-
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
Introduce PolymorphismStyle.None #494
Conversation
This implements charleskorn#378 by introducing a new `None` style that omits the tag/property entirely from the output yaml. Note this is asymmetric and reads are treated the same as `Tag` as reads require a tag. I added a few tests to JvmYamlWritingTest for both this case and some added coverage for the existing two cases as well.
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 for the PR @ZacSweers!
Overall, this looks good to me.
In addition to the comment below, could you please also add a test for the behaviour when reading polymorphic and non-polymorphic content while PolymorphismStyle.None
is in use?
PolymorphismStyle.Tag -> throw MissingTypeTagException(node.path) | ||
PolymorphismStyle.Tag, PolymorphismStyle.None -> throw MissingTypeTagException(node.path) | ||
PolymorphismStyle.Property -> createPolymorphicMapDeserializer(node, yaml, context, configuration) | ||
} | ||
else -> throw IncorrectTypeException("Expected ${descriptor.kind.friendlyDescription}, but got a map", node.path) | ||
} | ||
|
||
is YamlTaggedNode -> when { | ||
descriptor.kind is PolymorphicKind && configuration.polymorphismStyle == PolymorphismStyle.Tag -> YamlPolymorphicInput(node.tag, node.path, node.innerNode, yaml, context, configuration) | ||
descriptor.kind is PolymorphicKind && configuration.polymorphismStyle != PolymorphismStyle.Property -> { | ||
YamlPolymorphicInput(node.tag, node.path, node.innerNode, yaml, context, configuration) | ||
} |
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 might be simpler to just fail if we encounter polymorphism or a tag and PolymorphismStyle.None
is in use.
Thank you for working on this! I really appreciate it :) |
Will look when I'm back from vacation 👍 |
Done! |
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 @ZacSweers! I've got some minor nits for the test names, but I'll update the branch and then merge this - thanks again for the PR.
I think there's a bug that I missed in kaml/src/jvmMain/kotlin/com/charleskorn/kaml/YamlOutput.kt Lines 175 to 177 in 2f0de20
I think in the None case, it should just omit the tag right? If so, I can PR a quick fix |
That makes sense to me - a PR with a test case and fix would be great. |
This implements #378 by introducing a new
None
style that omits the tag/property entirely from the output yaml.Note this is asymmetric and reads are treated the same as
Tag
as reads require a tag.I added a few tests to JvmYamlWritingTest for both this case and some added coverage for the existing two cases as well.