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

Add support for deserialization Duration in HOCON format #2073

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

alexmihailov
Copy link
Contributor

Currently kotlin.time.Duration by default supports deserialization from ISO-8601-2 format. But HOCON uses a different format.
Made changes so that when deserializing kotlin.time.Duration from HOCON, by default use the format specified in the documentation.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the PR is good, but I have several thoughts:

  • Hocon format also supports encoding to config (Hocon.encodeToConfig). Do you plan to support Duration in encoding, too? If so, how will it be encoded? Milliseconds only?
  • Documentation should be updated to reflect this change. Since we do not have a separate HOCON entry in https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/serialization-guide.md, KDoc update should do the job.
  • Does it make sense to add a flag to restore old behavior? If one wants now to serialize Duration in ISO8601, they would have to resort to cumbersome workarounds like making a custom serializer that delegates to duration serializer but uses a different descriptor. However, I'm not well aware of hocon usages and if no one does ISO Duration serialization, I'm fine with not having flag.

@ihostage
Copy link

Hi, Leonid @sandwwraith!

  • Hocon format also supports encoding to config (Hocon.encodeToConfig). Do you plan to support Duration in encoding, too? If so, how will it be encoded? Milliseconds only?

Yes, we discussed this with Alex @alexmihailov and decided to implement this in separate PR later. You are right, it will be something like we implemented this case In Config4K
https://github.com/config4k/config4k/blob/20294740a3735851e20287bdf38874b2bc1f6b63/src/main/kotlin/io/github/config4k/Extension.kt#L97
In this case, it's hard to come up with something striking better 🤷‍♂️

  • Does it make sense to add a flag to restore old behavior? If one wants now to serialize Duration in ISO8601, they would have to resort to cumbersome workarounds like making a custom serializer that delegates to duration serializer but uses a different descriptor. However, I'm not well aware of hocon usages and if no one does ISO Duration serialization, I'm fine with not having flag.

I think it's not worth it. Typesafe Config as HOCON reference implementation doesn't support ISO8601 and I've never seen that someone use this format for Duration 😄 I think we will be able to add a flag by request in the future if I'm wrong and it's really needed for someone.

@sandwwraith
Copy link
Member

I think it's not worth it. Typesafe Config as HOCON reference implementation doesn't support ISO8601 and I've never seen that someone use this format for Duration

OK, no problem

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from minor documentation rewording, I do not have suggestions. However, I kindly ask you to submit encoding PR before the next release (which should happen in about a month), so the format continues to look consistent.

@alexmihailov
Copy link
Contributor Author

Apart from minor documentation rewording, I do not have suggestions. However, I kindly ask you to submit encoding PR before the next release (which should happen in about a month), so the format continues to look consistent.

For encoding, I already have a solution, I will submit a PR soon

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, great job!

@sandwwraith sandwwraith merged commit 77c8232 into Kotlin:dev Oct 27, 2022
fred01 pushed a commit to fred01/kotlinx.serialization that referenced this pull request Nov 24, 2022
Currently **kotlin.time.Duration** by default supports deserialization from **ISO-8601-2** format. But **HOCON** uses a different [format](https://github.com/lightbend/config/blob/main/HOCON.md#duration-format).
Made changes so that when deserializing **kotlin.time.Duration** from **HOCON**, by default, use the format specified in the documentation.
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