-
-
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 commutative apply and applicative to Const #2204
Conversation
Insufficient memory, but the changes should pass tests and MiMa |
Could it be possible to run the CI on this again ? Thanks. |
Codecov Report
@@ Coverage Diff @@
## master #2204 +/- ##
=======================================
Coverage 94.83% 94.83%
=======================================
Files 333 333
Lines 5789 5789
Branches 222 211 -11
=======================================
Hits 5490 5490
Misses 299 299
Continue to review full report at Codecov.
|
|
||
{ | ||
implicit val setMonoid: CommutativeMonoid[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.
Should we name this intCommutativeMonoid
instead, maybe?
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.
ops ... fixing it 👍
|
||
{ | ||
implicit val setMonoid: CommutativeSemigroup[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.
Rename this too?
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.
fixing this as well, thanks
|
||
{ | ||
implicit val intCommutativeMonoid: CommutativeMonoid[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.
Instead of defining this instance just for this test, I think that you should be able to use CMono
from Helpers.scala
. You can see an example of it being used in FunctionSuite.scala
. Similarly for the commutative semigroup below (CSemi
).
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.
sure, I will change it 👍
|
||
{ | ||
implicit val iso = SemigroupalTests.Isomorphisms.invariant[Const[CSemi, ?]](Const.catsDataFunctorForConst) |
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.
Is this iso
needed?
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.
obviously not 😄 . Thanks for this review. It's amazing to have your help.
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 can't understand where the Invariant[Const[CSemi, ?]]
comes from though. If is not too long, could you explain me why I have in scope that one but not Invariant[Const[CMono, ?]]
? Thanks
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.
To be honest I'm not familiar with why these SemigroupalTests.Isomorphism.invariant
s are needed in some tests. Maybe someone else can provide insight there. I wasn't sure why it was needed so I tried removing it in both cases and one of them still compiled ¯\_(ツ)_/¯
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.
Thanks a lot 👍
Thanks a lot, @barambani! One minor remaining question and then this looks good to me. |
I will fix it. Thank you for the time you spend on 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.
👍 thanks , @barambani!
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.
Thanks very much! :)
Thank you both 😄 |
This addresses the issue #2201