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

Add ability to control type encoding #5

Closed
wants to merge 13 commits into from

Conversation

notxcain
Copy link
Contributor

Sometimes it is needed to change how type is being encoded in json result. So I implemented it using implicit CoproductContainer parameter. Previous behavior is persisted. So it’s non breaking change.

@alexarchambault
Copy link
Owner

So I thought about all that. I think I'd prefer not to introduce an intermediary implicit (CoproductContainer).

I began to refactor argonaut-shapeless so that it would be easier to generate codecs that behave a bit differently than the default ones, see
https://github.com/alexarchambault/argonaut-shapeless/commits/topic/variedencodings,
and

import GenericEncodeJsons.{
hnilJsObjectEncodeJson, hconsJsObjectEncodeJson,
cnilEncodeJsonFails, cconsJsObjectEncodeJson,
defaultInstanceEncodeJson
}
import GenericDecodeJsons.{
hnilLooseJsObjectDecodeJson, stopAtFirstErrorHConsJsObjectDecodeJson,
cnilDecodeJsonFails, cconsAsJsObjectDecodeJson,
defaultInstanceDecodeJson
}

in particular. I think it should be possible to achieve what you'd like by supplying different implicits to encode ccons cells. That would amount to replace cconsJsObjectEncodeJson and cconsAsJsObjectDecodeJson by custom implicits in the former link.

These changes would be for argonaut-shapeless 0.3 (I'll publish snapshot versions as soon as shapeless 2.2 final is out). Would it be ok for you to do it this way?

I'd also be keen to add extra codecs, but the one you added in the tests doesn't feel general enough to me for that. I was more thinking about the same without the type field. I'd be ok to add this in https://github.com/alexarchambault/argonaut-shapeless/commits/topic/variedencodings. (But this may raise issues with the order of items in LabelledGeneric of sum types, and these may have to be addressed in shapeless directly...)

@notxcain
Copy link
Contributor Author

notxcain commented Apr 6, 2015

Yep importing different cconsJsObjectEncodeJson is a pretty straight forward, but imho is very low level for library consumer.

@alexarchambault
Copy link
Owner

I was thinking ultimately about putting alternate sets of implicits like these in other objects whose content could be imported instead of Shapeless._ (or along with it, with careful implicits shadowing), like import Shapeless.InlineSum._.

But for truly custom ways of handling sum types, it has to be a bit more complicated...
Actually, I'm now thinking it could be nice to have something a bit like your PR along with what I'm proposing: one would import Shapeless.Custom._ instead of Shapeless._, and possibly define some implicits JsonProductCodec or JsonCoproductCodec to override the default behaviour.

@alexarchambault
Copy link
Owner

I think the changes should consist in:

  • defining a JsonCoproductCodec trait instead of CoproductContainer, looking like:
trait JsonCoproductCodec {
  def encode(typeName: String, json: Json): Json
  def attemptDecode[T](typeName: String, decodeJson: DecodeJson[T], json: Json): Option[DecodeResult[T]]
}

(which changes a little the logic of decoding compared to your CoproductContainer),

  • defining an implicit default JsonCoproductCodec in the companion object that behaves like the current coproduct codecs,
  • defining new implicit methods in GenericDecodeJsons / GenericEncodeJsons which would use JsonCoproductCodec, that one would have to use instead of cconsAsJsObjectDecodeJson / cconsJsObjectEncodeJson,
  • defining CustomGenericDecodeJsons and CustomGenericEncodeJsons traits that would use these newly defined methods,
  • defining a Custom object in Shapeless that mixes these two traits,
  • and finally adapting the tests you added in your PR to these changes.

These are pretty big changes (which can be discussed). Tell me if you'd prefer me to take care of them.

@alexarchambault
Copy link
Owner

About the changes in JsonCoproductCodec compared to CoproductContainer, they allow for more general codecs, in particular the JsonCoproductCodec instance doesn't have to provide the name of the type being decoded.

@notxcain
Copy link
Contributor Author

notxcain commented Apr 6, 2015

Nice! I was thinking of example when I may need to override JsonProductCodec and here is one:
I may want an instance A(None) of case class A(oi: Option[Int]) to be encoded as {} instead of {"oi":null} and then decode it back.

Regarding refactoring. I think I can handle that. Gonna take some time though. Like until this friday.

@alexarchambault
Copy link
Owner

Great! I may throw away the basis of JsonProductCodec (and let JsonCoproductCodec and its details for you) in the mean time then.

@alexarchambault
Copy link
Owner

About the application of JsonProductCodec, there's this kind of things, yes, and I was also thinking about "case translations", like camelCase on the Scala side <-> serpent_case on the JSON side.

@notxcain
Copy link
Contributor Author

notxcain commented Apr 6, 2015

Nice one too!

@notxcain
Copy link
Contributor Author

@alexarchambault Hi! I'm kinda confused about how CustomGenericDecodeJsons and CustomGenericEncodeJsons traits would use these newly defined methods. Could you please take a look?

@notxcain
Copy link
Contributor Author

The obvious part is that these methods should be reused in both Custom and Automatic variants.

@alexarchambault
Copy link
Owner

Not much time for a detailed answer now, but:

  • thanks for your efforts :-)
  • were you looking at the topic/variedencodings branch? https://github.com/alexarchambault/argonaut-shapeless/tree/topic/variedencodings JsonCoproductCodec should fit there.
  • then I think it's a matter of adding ccons... methods in the CustomGeneric*Jsons singletons, which should be the counterpart of the hcons... methods already there, and have the ccons... methods in the traits below them use these newly added methods,
  • and adding a few tests too.

Maybe you should just submit a new PR against topic/variedencodings (with fewer commits too). Or if you don't have time, it may be possible for me to just cherry-pick some of your commits in this PR. I'll try (not now, but soon).

@notxcain
Copy link
Contributor Author

That's detailed enough ) thanks, I got it! Will do!

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.

2 participants