eed3si9n / sjson-new

a typeclass based JSON codec that's backend independent
Apache License 2.0
36 stars 19 forks source link

Fixes LListFormats #75

Closed eed3si9n closed 7 years ago

eed3si9n commented 7 years ago

This is a continuation of https://github.com/eed3si9n/sjson-new/pull/74 by @dwijnand

When we detect an elided field (a field that's in the "fields" list, but missing from JSON object), the unbuilder will now return JNull. This should be fine because we also encode Option[A] by not writing out the field when possible, and there's no special meaning attached to JSON null.

dwijnand commented 7 years ago

We're throwing away here the different between a field missing and a field being set to null.

eed3si9n commented 7 years ago

sjson-new already doesn't distinguish them in OptionFormat.

eed3si9n commented 7 years ago

In the future when we can break bincompat we could return Option[J] in Unbuilder, but using JNull I think is a much better workaround than using Java null.

dwijnand commented 7 years ago

I'm saying for any consumer of Unbuilder, both in and out if this repo, not just OptionFormat; it's kind of evident from your "Increase tolerance to JNull" commit.

dwijnand commented 7 years ago

Btw, I'm not saying that null is a great fix. But I think it's the better fix because it keeps the distinction between a field missing and a field being set, in JSON, to 'null'.

And it lends itself better to when we fix this properly to return Option.

eed3si9n commented 7 years ago

I'm saying for any consumer of Unbuilder, both in and out if this repo, not just OptionFormat; it's kind of evident from your "Increase tolerance to JNull" commit.

This situation only comes up if you ever pass in explicit list of field names. The only user that does such a thing is us decoding LList.

dwijnand commented 7 years ago

... and FlatUnionFormats (which btw we want to check for when empty collections are written, because I see a deserializationError("Field not found: $type") in there).

Let's discuss the other side: what reasons do you have to use JNull over null?

eed3si9n commented 7 years ago

Java null is just bad, especially exposed out to the public API. We can reasonably ask people to handle JNull, which is part of the JSON spec, whereas telling people that some method might occasionally return Java null is difficult to debug.