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

Fixes LListFormats #75

Merged
merged 5 commits into from
Jul 26, 2017
Merged

Fixes LListFormats #75

merged 5 commits into from
Jul 26, 2017

Conversation

eed3si9n
Copy link
Owner

@eed3si9n eed3si9n commented Jul 24, 2017

This is a continuation of #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
Copy link
Collaborator

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

@eed3si9n
Copy link
Owner Author

eed3si9n commented Jul 24, 2017

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

@eed3si9n
Copy link
Owner Author

eed3si9n commented Jul 24, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

... 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
Copy link
Owner Author

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.

case (k, Some(v)) => (k, v)
case (k, None) => (k, facade.jnull())
}
case ctx: UnbuilderContext.ObjectContext[J] => ctx.next
Copy link
Owner Author

Choose a reason for hiding this comment

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

@dwijnand Not sure if this is what you were talking about, but no more null or JNull with this solution.

Copy link
Collaborator

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Opt is not what I meant, but it's the even better solution. Man, we've spent so much time with the liberty of breaking APIs that our bincompat game is weak!

Lgtm

if (!unbuilder.isInObject) objectPreamble(js)
if (unbuilder.hasNextField) {
val (name, optX) = unbuilder.nextFieldOpt
optX map { x =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

foreach over map

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

@eed3si9n eed3si9n merged commit bdc7efa into master Jul 26, 2017
@eed3si9n eed3si9n deleted the wip/fix-llist-roundtrip branch July 26, 2017 21:20
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.

None yet

2 participants