-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added Align
instances for Id
and Kleisli
#4287
Conversation
@@ -89,6 +89,12 @@ class KleisliSuite extends CatsSuite { | |||
SerializableTests.serializable(CommutativeMonad[Kleisli[Id, Int, *]]) | |||
) | |||
|
|||
{ | |||
implicit val K: Align[Kleisli[Id, MiniInt, *]] = Kleisli.catsDataAlignForKleisli[Id, MiniInt] |
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.
For some reason this is required for scala 2.12.x, as it fails to find the implicit Align for Kleisli on its own. Is there another way to do this?
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.
I guess it must be an inference issue. /shrug
[error] /workspace/cats/tests/shared/src/test/scala/cats/tests/KleisliSuite.scala:93:51: could not find implicit value for evidence parameter of type cats.Align[[γ$23$]cats.data.Kleisli[[A]A,cats.laws.discipline.MiniInt,γ$23$]]
[error] checkAll("Kleisli[Id, MiniInt, *]", AlignTests[Kleisli[Id, MiniInt, *]].align[Int, Int, Int, Int])
[error] ^
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.
LGTM. So sorry for the slow review cycle.
def FA: Align[F] | ||
|
||
override def align[A, B](fa: Kleisli[F, R, A], fb: Kleisli[F, R, B]): Kleisli[F, R, Ior[A, B]] = | ||
Kleisli(r => FA.align(fa.run(r), fb.run(r))) |
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.
I think this is a stack-safety trap, but it's not obvious to me if there's a better way.
Align
instances for Id
and Kleisli
No worries, definitely not urgent, appreciate your dedication to OSS! Should I go ahead and merge or wait for more reviewers? |
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.
Looks good to me too, thanks!
This PR adds Align instances for Kleisli and Id.
I realized these two were not available while trying some code like (simplified):
Looking at docs and laws I see no reason why they shouldn't have it. Please let me know otherwise.