-
-
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
Add Semigroupal for Monoidal hierarchy, fixes #740 #795
Conversation
843d7c5
to
ed09428
Compare
Current coverage is
|
* or more) within a context, which can then be mapped over via the [[Functor]] | ||
* instance. | ||
*/ | ||
@typeclass trait Semigroupal[F[_]] extends Functor[F] { |
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 that’s not a good idea to restrict Semigroupal
to be a specialization of Functor
. For instance, you can not anymore implement Semigroupal[Show].
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.
Agreed, this would also prevent a Semigroupal[Codec]
.
I'm not really comfortable with the term I also don't think we should have type classes that are equivalent -- e.g., I don't think there's a need for both applicative and monoidal as they are represented in this PR. My preference is that we leave the canonical type class hierarchy: invariant ← functor ← apply ← applicative We then create a new type class: @typeclass trait Product[F[_]] {
def product[A, B](fa: F[A], fb: F[B]): F[(A, B)]
} We then change We then change the When creating an instance of |
I would rather see Semigroupal a typeclass which does not extend anything. Monoidal should extend it and when you equip it with |
@mpilquist Ah that sounds good, new PR coming in shortly.. in terms of |
Cartesian works for me. |
65cf576
to
d85cf4f
Compare
…oduct and ap consistency, fixes typelevel#740
Sorry @adelbertc this has some merge conflicts. |
@mpilquist @julienrf review pls :D |
new MonoidalBuilder[F] |@| self |@| fb | ||
abstract class CartesianOps[F[_], A] extends Cartesian.Ops[F, A] { | ||
def |@|[B](fb: F[B]): CartesianBuilder[F]#CartesianBuilder2[A, B] = | ||
new CartesianBuilder[F] |@| self |@| fb | ||
|
||
def *>[B](fb: F[B])(implicit F: Functor[F]): F[B] = F.map(typeClassInstance.product(self, fb)) { case (a, b) => b } | ||
|
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.
Maybe you should also rename the file to cartesian.scala
Hi, thanks for working on this topic! I appreciate the value of this PR, renaming
trait Monoidal[F[_]] extends Cartesian[F] {
def pure[A](a: A): F[A]
}
def monoidalLeftIdentity[A](fa: F[A]): ((F[Unit], F[A]), F[A]) =
(F.product(F.pure(()), fa)), fa)
def monoidalRightIdentity …
If needed, we could capture trait Pure[F[_]] {
def pure[A](a: A): F[A]
} And then define trait Monoidal[F[_]] extends Cartesian[F] with Pure[F] I don’t remember if this is needed or not. |
I’m OK for this work to be merged as it is, and to continue on working on it as per #817 |
@mpilquist @ceedubs review? :D |
trait Isomorphisms[F[_]] { | ||
def associativity[A, B, C](fs: (F[(A, (B, C))], F[((A, B), C)]))(implicit EqFABC: Eq[F[(A, B, C)]]): Prop | ||
def leftIdentity[A](fs: (F[(Unit, A)], F[A]))(implicit EqFA: Eq[F[A]]): Prop | ||
def rightIdentity[A](fs: (F[(A, Unit)], F[A]))(implicit EqFA: Eq[F[A]]): Prop |
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 is not very important, but I’m currently reading this paper and it seems that the above isomorphisms should be named associator
, leftUnitor
and rightUnitor
, to be consistent with the literature.
@adelbertc sorry, I don't think I'm qualified to review this one. I'm hoping @mpilquist will weigh in. When you, @julienrf, and @mpilquist are all happy with it, then I think it's good to merge. |
👍 |
@adelbertc Let's merge after the conflicts are fixed. |
Merge conflicts resolved, will merge when Travis gives the green light. |
Add Semigroupal for Monoidal hierarchy, fixes #740
So while I was doing this it has become even more apparent that we run into some strange-ness having both
Apply/Applicative
andSemigroupal/Monoidal
. Currently we haveApply extends Monoidal
(nowSemigroupal
) so we sort of give special treatment toApply
which doesn't particular make sense to me. Should we delete one or the other?/cc @julienrf who did the initial work on
Monoidal
and @mpilquist who is our resident expert :-)