-
Notifications
You must be signed in to change notification settings - Fork 624
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
Hocon encoder #1740
Conversation
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/HoconEncoder.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
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.
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 :) |
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconPolymorphismTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/HoconEncoder.kt
Outdated
Show resolved
Hide resolved
Thank you for your quick response!
Added some use-cases to the PR description. |
@qwwdfsad sorry for bothering you but is there anybody who is able to review this PR? |
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 overall changes look good, thanks a lot for the effort! I have a few pretty minor comments and it will be good to go.
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Outdated
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconEncoderTest.kt
Show resolved
Hide resolved
formats/hocon/src/test/kotlin/kotlinx/serialization/hocon/HoconRootObjectsTest.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.
Great work, thanks for the contribution and your patience!
formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt
Outdated
Show resolved
Hide resolved
import kotlinx.serialization.* | ||
import org.junit.* | ||
|
||
class HoconEncoderTest { |
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.
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.
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.
Ok, I'll add it
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.
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?
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'll try to finish it ASAP
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.
Added tests for unsupported root objects and map keys in two last commits.
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.
Great job, thanks for all your efforts!
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
Essentials
useConfigNamingConvention
supportNice to have
encodeDefaults
supportJsonClassDiscriminator
Closes #1609