-
Notifications
You must be signed in to change notification settings - Fork 123
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 TypeclassBijection #183
Conversation
I accidentally forgot to include the TypeclassBijection instance necessary in the test... added it, and still get the divergence. |
Ack, missing the actual bijection. Heh, been staring at this stuff too long. Will add it shortly. |
Ok, added the bijection. If you take a look, this line works
whereas this one does not
One possible strategy we might do is to expose a type specific transformer per type class, instead of one global one. I love the idea of having a global one (if it works!), but it's not too onerous to have somthing like
Or something like that. This way libraries can leverage it without the perhaps heavyweight implicit in the scope of bijection. Or, and this is my preference, we can try to get this working ;) Any thoughts on what might be failing, @travisbrown ? |
Hmm, that's surprising to me. I'll take a look over lunch. |
Would love to see if you can figure it out. I still think an implicit that does this would be useful, though it probably shouldn't be a default. I thought about this a bit (and chatted with @mosesn though what follows isn't endorsed by him) If you look at the tests, you'll see the main two methods:
I think this is a good way for this to work. It's totally generic, but it provides the main cases we want. bijectTo will change the type parameter on a type class assuming that the TypeclassBijection is in scope, as well as Bijection[From, To] deriveFor will do a best effort to get a meaningful typeclass for you. It hinges on a Bijection[Type, Whatever] being in scope, with there being a typeclass for whatever. This is nice because in the case of the macro generated conversions, you can get nasty stuff like Look forward to your thoughts. |
Are there supposed to be macros here? How is this working? |
@johnynek no macros, at least, not this piece. Take a look at TypeclassBijection.scala and TypeclassBijectionLaws.scala (the latter of which will be much refined once we settle on design, but does show off the syntax for now and tests that it compiles). This lays the groundwork, and then with the macros, it means we can autoderive any type class for a case class, assuming a TypeclassBijection is defined. |
package com.twitter.bijection | ||
|
||
object TypeclassBijection { | ||
implicit def toRichTypeclass[T[_], A](t: T[A]): RichTypeclass[T, A] = RichTypeclass(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implicit class is the way to go on these things now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Seems like a good idea to me. I know shapeless has a Typeclass trait, I think, but I don't know much about it. @travisbrown can probably comment on the relationship. |
d377d8a
to
c33e9dd
Compare
I'm a little concerned about supporting things like Group[Z_4] => Group[Z_2 x Z_2]. Should we allow an automatic conversion between these two groups even though they're not isomorphic? Not all typeclasses will benefit from this kind of thing. This could also be my lack of math knowledge showing, not sure. |
@mosesn, I actually agree with you entirely. That's why I think making it not implicit is superior. The user has to ask for this to happen, it won't happen to them... so it's up to them to reason about correctness. This mirrors how ordering.on works, for example... it's entirely possible to make a bad ordering with that function, but for reasonable cases, it's fine. But I agree that if we made it implicit it could be dangerous and cause difficult to reason about bugs. |
Shapeless's |
@johnynek where do you want to set the bar for contributing something like this? I think it's a good base to build off of with the macro work, and isn't intrusive (ie not wacky implicits). If I add more tests are we good, or is there something in the design you want to refine? What do you think, @travisbrown ? |
case class A(x: Int, y: String) | ||
implicit val abij: Bijection[A, (Int, String)] = Bijection.build[A, (Int, String)] { A.unapply(_).get } { x => A(x._1, x._2) } | ||
|
||
implicit val orderingTypeclassBijection: TypeclassBijection[Ordering] = new TypeclassBijection[Ordering] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in TypeclassBijection if you ask me. This will be common one. Also Equiv would be interesting there.
Do this, and I think we should merge.
This is in response to the conversation in #182. It has no dependence on macros (though if we can get it working, they will make it much more useful!).
Either way, if you try to run the tests, you'll see that they diverge.
./sbt "bijection-core/testOnly com.twitter.bijection.TypeclassBijectionLaws"
returns
This is the error that Derived was trying to work around.. I don't think the TypeNotEquals is necessary, but maybe that's the issue. Hm. Any ideas, @travisbrown ?