-
-
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 InvariantSemigroupal and ability to turn Monoidals to Monoids #2088
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2088 +/- ##
==========================================
+ Coverage 94.63% 94.66% +0.03%
==========================================
Files 325 328 +3
Lines 5514 5533 +19
Branches 221 199 -22
==========================================
+ Hits 5218 5238 +20
+ Misses 296 295 -1
Continue to review full report at Codecov.
|
def G: Apply[G] | ||
|
||
def product[A, B](fa: F[G[A]], fb: F[G[B]]): F[G[(A, B)]] = | ||
F.imap(F.product(fa, fb)) { case (ga, gb) => |
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.
do you plan to add tests for this instance?
*/ | ||
@typeclass trait InvariantSemigroupal[F[_]] extends Semigroupal[F] with Invariant[F] { self => | ||
|
||
def composeApply[G[_]: Apply]: InvariantSemigroupal[λ[α => F[G[α]]]] = |
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.
Any plan to test cover this one as well?
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.
Has this been addressed?
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.
Yep!
} | ||
|
||
object InvariantSemigroupal extends SemigroupalArityFunctions { | ||
def semigroup[F[_], A](implicit f: InvariantSemigroupal[F], sg: Semigroup[A]): Semigroup[F[A]] = |
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.
The diff coverage is a bit low, one thing we could do is to add doctests for this semigroup
method and the one in ContravariantSemitgroupal
, which could also serve as examples. Not critically important though.
Overall looking good to me. Could use a couple of more tests for the composed instances to bring up the test coverage. |
Yeah this is still a WIP, wanted to add a bunch more tests, for Applicative/Apply as well. |
6d44ab1
to
bd825fe
Compare
bd825fe
to
19dc0ff
Compare
The more I work with this the more I think |
I moved the |
2e12089
to
3a1f937
Compare
3a1f937
to
f4b7490
Compare
So the version right now works without breaking changes, but it'd probably be cleaner to define |
Then |
Nope, that's no problem, in fact if you look at trait Applicative[F[_]] extends Apply[F] with InvariantMonoidal[F] {
def pure[A](a: A): F[A]
def unit: F[Unit] = pure(())
} https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Applicative.scala#L46 |
hmm, I admit I am a bit confused, in |
It would have to be changed or renamed, yeah :/ Something like: trait ContravariantMonoidal[F[_]] extends ContravariantSemigroupal[F] with InvariantMonoidal[F] {
def conquer[A]: F[A]
def unit: F[Unit] = conquer[Unit]
} Or we just remove it, since you can |
Another argument I find compelling is that literature always only refers to |
Everything should be addressed :) |
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. Codecov is reporting a drop in coverage which might be worth checking out.
F.imap(F.product(fa, fb)) { case (ga, gb) => | ||
G.map2(ga, gb)(_ -> _) | ||
} { g: G[(A, B)] => | ||
(G.map(g)(_._1), G.map(g)(_._2)) |
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 line is not tested, we might need to test an instance that is not a covariant.
override implicit def F: InvariantMonoidal[F] | ||
import cats.syntax.semigroupal._ | ||
import cats.syntax.invariant._ | ||
|
||
def invariantMonoidalLeftIdentity[A, B](fa: F[A], b: B): IsEq[F[A]] = | ||
F.pure(b).product(fa).imap(_._2)(a => (b, a)) <-> fa | ||
F.point(b).product(fa).imap(_._2)(a => (b, a)) <-> fa |
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.
We can express these laws in unit
right? like
def invariantMonoidalLeftIdentity[A, B](fa: F[A]): IsEq[F[A]] =
F.unit.product(fa).imap(_._2)(a => ((), a)) <-> fa
Then we can add a point
- unit
consistency law. WDYT?
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!
@@ -136,7 +136,7 @@ private[data] sealed abstract class ConstInstances0 extends ConstInstances1 { | |||
|
|||
private[data] sealed abstract class ConstInstances1 { | |||
implicit def catsConstInvariantMonoidal[C: Monoid]: InvariantMonoidal[Const[C, ?]] = new InvariantMonoidal[Const[C, ?]] { | |||
def pure[A](a: A): Const[C, A] = | |||
def unit: Const[C, Unit] = | |||
Const.empty |
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 one is not tested. I suspect that the instance was never tested before?
override def unit[A]: IndexedStateT[F, S, S, A] = | ||
IndexedStateT.applyF(G.pure((s: S) => F.unit[(S, A)])) | ||
override def unit: IndexedStateT[F, S, S, Unit] = | ||
IndexedStateT.applyF(G.pure((s: S) => F.trivial[(S, Unit)])) |
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.
It's reported that this instance is not tested. Shall we add one?
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.
the ContravariantMonoidal
instance of IndexedStateT
requires that F
to have both FlatMap
and ContravriantMonoidal
. We can't think of any practical data types that satisfies that.
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 incline to remove this instance, WDYT @stephen-lazaro ?
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, it seems at best awkward and unuseful.
@@ -8,7 +8,7 @@ trait EqInstances { | |||
* Defaults to the trivial equivalence relation | |||
* contracting the type to a point | |||
*/ | |||
def unit[A]: Eq[A] = Eq.allEqual | |||
def unit: Eq[Unit] = Eq.allEqual |
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.
not quite sure I understand why this one is reported as not tested either. Any idea?
Luka jcb add monoidal to monoid
Sorry @kailuowang, needed to add a couple more mima exceptions |
close and reopen to trigger build and recheck of conflict |
Adds
InvariantSemigroupal
as supertype ofInvariantMonoidal
and just like withApply
andApplicative
we have functions that turn invariant and contravariantMonoidal
andSemigroupal
Functors intoMonoids
andSemigroup
s respectively.I'll try to add tests for these, as well as the ones in
Apply
andApplicative
soon.