julienrf / play-json-derived-codecs

MIT License
191 stars 34 forks source link

Adding support for using plain `Writes`/`Format` as custom implicits. #79

Closed ncreep closed 3 years ago

ncreep commented 3 years ago

When using custom formats with the the nested format it's now possible to use any Writes/Format.

The flat format still requires an OWrites/OFormat type.

Partially addresses #76.

This change required duplicating the content of the withTypeTag object, so that now we have withTypeTag.flat. Hopefully, this change won't be noticed by most users (as using withTypeTag with non-default arguments is probably rare).

julienrf commented 3 years ago

Hey @ncreep thank you for the PR! I think your solution is rather elegant, implementation-wise. However, I’m a bit worried by all the things we need to take care of to correctly provide custom formats that work as expected, and by the complexity the PR adds to the current design (which is already quite complex, IMHO).

ncreep commented 3 years ago

Seeing how I'm responsible for introducing the weird behavior with user-provided formats, I felt responsible to try and do something about it.

I can see your point about the complexity this adds, but the edge cases with mismatched/ignored formats are rather unpleasant. The question is whether the current behavior as described in #76 is acceptable.

The alternative solution that I can see, was the one I suggested in #76, lifting the restriction on TypeTagOWrites to work with any Writes. That would require zero added complexity, but will introduce an edge case for the flat formats.

Or, worst case for my purposes, is to completely remove support for custom formats this way, and revert to the behavior before I started breaking things...

julienrf commented 3 years ago

The alternative solution that I can see, was the one I suggested in #76, lifting the restriction on TypeTagOWrites to work with any Writes. That would require zero added complexity, but will introduce an edge case for the flat formats.

If I understand correctly, we would not be able to detect, at compile-time, that someone tries to use a Write with flat, so the best we could do in that case would be to fail at run-time. Am I right?

I prefer what we have in this PR, if that’s the case.

Or, worst case for my purposes, is to completely remove support for custom formats this way ,and revert to the behavior before I started breaking things...

I think it’s a valuable feature, I wouldn’t drop it like that. I just wanted to take some time to brainstorm a little bit more about possible alternative solutions…

ncreep commented 3 years ago

I'm not aware of a technique to detect the Write/flat problem at compile-time (detecting what's being provided in the regular scope vs. generated is too deep for me, though it's probably possible with more type-tagging and added complexity). So I can only think of runtime behavior. But it doesn't have to be failure. As mentioned in my original issue, it's possible to fall back to a synthetic wrapper in that case. So if you have:

sealed trait Foo
case class Bar(x: Int) extends Foo

object Bar { implicit val format: Format[Bar] = Json.valueFormat }
object Foo { implicit val format: Format[Foo] = derived.flat[Foo]() }

Then the resulting JSON for Bar(42) would be something like:

{ "type": "Bar", "value": 42 }

Where the value field was auto-generated, never defined by the user, and doesn't really match the idea of being flat. That may be a surprising edge case, but I would personally prefer that over the mismatched and failing Reads/Writes.

julienrf commented 3 years ago

That may be a surprising edge case, but I would personally prefer that over the mismatched and failing Reads/Writes.

Interesting, I didn’t think about that idea. I think that could be a good compromise that would reduce the complexity in play-json-derived-codecs, and offer a reasonable behavior (it needs to be properly documented, though). Would you be interested in changing to that?

ncreep commented 3 years ago

Yes, I will try to come up with a new pull request in the coming days.

Thanks

ncreep commented 3 years ago

Opened another pull request (#81)