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

Hocon encoder #1740

Merged
merged 29 commits into from Dec 23, 2021
Merged

Hocon encoder #1740

merged 29 commits into from Dec 23, 2021

Conversation

osipxd
Copy link
Contributor

@osipxd osipxd commented Oct 22, 2021

Just started to work on Hocon encoder implementation and want to make my work "transparent" to kotlinx.serialization team.
If this feature is not needed I'll stop working on it, otherwise, any comments are welcome!

Use cases

  • Generate default config. For now, it is possible only to provide default config files from resources.
  • Edit config from an app. This feature might be useful for apps having both text config and UI for configuration.

Essentials

  • Primitives encoding
  • Lists encoding
  • Maps encoding
  • Nested objects encoding
  • Polymorphic types encoding
  • useConfigNamingConvention support
  • Reasonable exception messages
  • Add KDoc comments

Nice to have

  • encodeDefaults support
  • Annotation like JsonClassDiscriminator
  • Add description (origin) to config values

Closes #1609

@osipxd osipxd marked this pull request as draft October 23, 2021 06:53
@osipxd osipxd marked this pull request as ready for review October 23, 2021 14:19
@qwwdfsad
Copy link
Member

Thanks! We're currently busy with an upcoming release of Kotlin and Kover, so will likely return to your PR shortly after the 9th of November.

If this feature is not needed

The feature is welcomed and it seems like at least a few people wanted that. It's good to start any development with a use-case though :)

@osipxd
Copy link
Contributor Author

osipxd commented Oct 26, 2021

Thank you for your quick response!

It's good to start any development with a use-case though :)

Added some use-cases to the PR description.

@osipxd osipxd changed the base branch from master to dev October 31, 2021 10:30
@osipxd osipxd requested a review from qwwdfsad October 31, 2021 11:01
@osipxd
Copy link
Contributor Author

osipxd commented Nov 16, 2021

We're currently busy with an upcoming release of Kotlin and Kover, so will likely return to your PR shortly after the 9th of November.

@qwwdfsad sorry for bothering you but is there anybody who is able to review this PR?

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

The overall changes look good, thanks a lot for the effort! I have a few pretty minor comments and it will be good to go.

@osipxd osipxd requested a review from qwwdfsad November 22, 2021 18:36
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Great work, thanks for the contribution and your patience!

import kotlinx.serialization.*
import org.junit.*

class HoconEncoderTest {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I missed that, but I don't see the test that checks that serializing an Int, List, etc. leads to a SerializationException("Value of type '${configValue.valueType()}' can't be used at the root of HOCON Config."). Can you add it, please? And for other exception cases too, if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll add it

Copy link
Member

Choose a reason for hiding this comment

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

It seems that work is almost finished; can you add this test (and resolve conflicts with #1795) so we can include your work in the release planned for this week?

Copy link
Contributor Author

@osipxd osipxd Dec 20, 2021

Choose a reason for hiding this comment

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

I'll try to finish it ASAP

Copy link
Contributor Author

@osipxd osipxd Dec 23, 2021

Choose a reason for hiding this comment

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

Added tests for unsupported root objects and map keys in two last commits.

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.

Great job, thanks for all your efforts!

@sandwwraith sandwwraith merged commit 261490a into Kotlin:dev Dec 23, 2021
@osipxd osipxd deleted the hocon-encoder branch December 23, 2021 14:53
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.

Encode an object into a Hocon config
3 participants