kotlinx-serialization support #101
Conversation
Additionally, check gzipped JSON and Protobuf in the "benchmarks".
* A new subproject was created just for testing. * DateTimePeriod serialization is not tested yet * Tests that serialize 64-bit numbers fail on nodejs for some reason * Enum classes serialization had to be implemented manually. Though https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/builtin-classes.md#enum-classes claims that enums are automatically serialized, on Kotlin/Native and Kotlin/JS runtime crashes occur if no serializer is passed explicitly. * Some code had to be modified due to a bug in the legacy JS codegen that led to an infinite hanging of deserialization.
|
|
||
| @ExperimentalSerializationApi | ||
| @Suppress("INVISIBLE_MEMBER") // to be able to throw `MissingFieldException` | ||
| override fun deserialize(decoder: Decoder): DateTimeUnit.DateBased.MonthBased { |
There was a problem hiding this comment.
Since this method is mostly identical for all serializers, I'd suggest to abstract it out to a single method like decodeStructureWithOneInt(descriptor, fieldName)
There was a problem hiding this comment.
I attempted such a refactoring and decided against it: I only found two serializer that share this code, the ones for DateTimeUnit.DateBased.MonthBased and DateTimeUnit.DateBased.DayBased; DateTimeUnit.TimeBased also looks very similarly, but it actually uses a Long.
There was a problem hiding this comment.
You can decode Long and that check if it fits into Int. Or I'm missing something?
There was a problem hiding this comment.
I don't know. Can I safely call decodeLongElement when the serial descriptor claims that the field is an Int?
There was a problem hiding this comment.
Think so. Descriptors are mainly used for scheme, and your scheme accepts ints
dkhalanskyjb
force-pushed
the
serialization-experiments
branch
from
fe34484 to
b3ce0fe
Compare
|
|
||
| } | ||
|
|
||
| object DatePeriodISO8601Serializer: KSerializer<DatePeriod> { |
There was a problem hiding this comment.
Not sure what you all use as a style guide, but I try really hard to evangelize the practice of treating acronyms and acronym-like things such as ISO, IPv6, iOS, etc. as words and casing them as such.
Google's Java and Kotlin style guide recommend this practice:
- https://google.github.io/styleguide/javaguide.html#s5.3-camel-case
- https://developer.android.com/kotlin/style-guide#camel_case
That would make this type be DatePeriodIso8601Serializer. And I think there's one or two more in the change.
There was a problem hiding this comment.
+1 for DatePeriodIso8601Serializer
There was a problem hiding this comment.
Agree, it would be consistent with Duration.toIsoString in stdlib, for example.
What do you think about dropping 8601 suffix? On one hand, it's not that there are many ISO standards on date representation, on the other hand, the serializer name is not supposed to be used often, so the character economy there may be not worth it.
core/common/src/Month.kt
Outdated
| @@ -20,7 +20,6 @@ public expect enum class Month { | |||
| OCTOBER, | |||
| NOVEMBER, | |||
| DECEMBER; | |||
|
|
|||
Fixes #37