Add null value handling for kotlin TypesAdapters #110
Conversation
|
@macisamuele @martinbonnin @cortinico Any thoughts on this PR? We spent a day and a half debugging this issue and @mikeyshean discovered that |
|
@mikeyshean thank a lot for the contribution 🎉 The pr looks sane to me, but I need some time to verify it. Something that for sure I'll ask is to have testing coverage over the change and ideally a unit test in samples/junit-test that ensures no further regressions around this. |
There was a problem hiding this comment.
As mentioned in the comment before we do need to have testing coverage over this change as a regression on this might lead again to the same bug.
I tried to play around to re-create a case similar to the one in #109 but I was not successful.
| @@ -60,7 +60,7 @@ internal class LocalDateAdapter : XNullableJsonAdapter<LocalDate>() { | |||
| override fun fromNonNullString(nextString: String): LocalDate = LocalDate.parse(nextString, formatter) | |||
|
|
|||
| override fun toJson(writer: JsonWriter, value: LocalDate?) { | |||
| value?.let { writer.value(it.format(formatter)) } | |||
| value?.let { writer.value(it.format(formatter)) } ?: writer.nullValue() | |||
There was a problem hiding this comment.
I would rather suggest to have concrete implementations dealing only with the "not-nullable" case and having XNullableJsonAdapter taking care of the null handling.
Something like
internal abstract class XNullableJsonAdapter<T> : JsonAdapter<T>() {
abstract fun fromNonNullString(nextString: String): T
abstract fun toJsonNonNull(writer: JsonWriter, value: T)
...
}This allows to focus the attention of nullable in the class related to nullable and to the T type encoding in the related adapter class
|
Any news on this? |
|
Sorry, the project we were using this on was canceled. @klaszlo8207 feel free to pick up this PR and add a regression test to it. |
PR for #109