-
-
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
SemigroupK sum: F[A], F[B] => F[A Either B] #3124
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3124 +/- ##
==========================================
+ Coverage 93.17% 93.23% +0.06%
==========================================
Files 372 375 +3
Lines 7182 7292 +110
Branches 199 194 -5
==========================================
+ Hits 6692 6799 +107
- Misses 490 493 +3
Continue to review full report at Codecov.
|
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.
Thank you! :)
* {{{ | ||
* scala> import cats.SemigroupK | ||
* scala> import cats.data.NonEmptyList | ||
* scala> SemigroupK[NonEmptyList].sum(NonEmptyList.one(1), NonEmptyList.one(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 is nitpicky, but what do you think about using different types for this example? Otherwise looks good to me.
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 was hesitant about using NonEmptyList here, but I wasn't sure about the alternatives either, so I stayed with NEL.
do you perhaps have any suggestions which type could be used here instead? :)
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.
@pk044 Oh, sorry, that wasn't very clear. I think NonEmptyList
is a good choice, just that using different values for A
and B
might make it a little clearer—e.g. something like:
SemigroupK[NonEmptyList].sum(NonEmptyList.one("abc"), NonEmptyList.one(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.
ok, i changed the value 👍
Closes #3112