-
-
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
Request to Include cats.data.Coproduct
and cats.free.Inject
#671
Conversation
This looks like a great PR! I'm on a train so I can't do a detailed review yet, but on the surface it looks great. Thanks for updating the tutorial and including tests as well! 🐈 |
This is awesome, thanks! Looks like there is a legitimate build failure - I'm guessing Other than that, lots of tests, lots of doc, I like it 👍 |
implicit def coproductSyntax[F[_], A](a: F[A]): CoproductOps[F, A] = new CoproductOps(a) | ||
} | ||
|
||
class CoproductOps[F[_], A](val a: F[A]) extends AnyVal { |
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.
Minor: let's go ahead and make this class final
to be consistent with other Ops
classes.
Thanks a bunch @raulraja. I left some minor comments, but in general this is very clean, has good test coverage, and even comes with a great tutorial! 💯 |
Current coverage is
|
@ceedubs @adelbertc @non addressed all your comments, let me know if anything else can be improved. thx! |
|
||
} | ||
|
||
sealed abstract class CoproductInstances3 { |
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.
Sorry for the minor details, but could you make the CodproductInstances*
traits private[data]
? That will make them consistent with the work done in #612.
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.
No worries, I want to get it right so I understand what the expectations are for future contributions.:) Added those access modifiers also on InjectInstances
…oproduct, A] where the coproduct can be used to compose dispair Free Algebras
@ceedubs @adelbertc @non Added |
checkAll("Coproduct[Option, Option, ?]", TraverseTests[Coproduct[Option, Option, ?]].traverse[Int, Int, Int, Int, Option, Option]) | ||
checkAll("Traverse[Coproduct[Option, Option, ?]]", SerializableTests.serializable(Traverse[Coproduct[Option, Option, ?]])) | ||
|
||
checkAll("Coproduct[Option, Option, ?]", FoldableTests[Coproduct[Option, Option, ?]].foldable[Int, Int]) |
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.
Unfortunately, I think this will pick up the same instances as the .traverse
check above. That's why I recommended the somewhat bizarre ListWrapper.foldable
approach in another comment.
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 see, I'll give that a shot
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.
@ceedubs Would this do it? non@5ea5313
…ope so they don't pick up those from Traverse and Comonad
👍 💯 |
👍 Looks good to me! |
👍 Thanks @raulraja , awesome PR |
Request to Include `cats.data.Coproduct` and `cats.free.Inject`
The following PR adds
cats.data.Coproduct
andcats.free.Inject
largely based on those found in Scalaz as in "Data types a la carte" (Swierstra 2008).Coproduct
&Inject
can be used to compose Free based Algebras where an Application is nothing but the Coproduct resulting from the interaction of it's algebras.In other words:
Interpreters omitted for simplicity but available in the free monads tutorial and Inject tests.
@non @ceedubs thanks for your consideration.