Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added CommutativeMonoid for Option to scope #3463

Merged
merged 2 commits into from
Jun 11, 2020
Merged

Added CommutativeMonoid for Option to scope #3463

merged 2 commits into from
Jun 11, 2020

Conversation

barambani
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #3463 into master will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3463      +/-   ##
==========================================
+ Coverage   91.60%   92.05%   +0.44%     
==========================================
  Files         382      382              
  Lines        8315     8316       +1     
  Branches      210      210              
==========================================
+ Hits         7617     7655      +38     
+ Misses        698      661      -37

djspiewak
djspiewak reviewed Jun 10, 2020
View reviewed changes
kernel/src/main/scala/cats/kernel/Semigroup.scala Outdated
implicit def catsKernelSemigroupForTry[A: Semigroup]: Semigroup[Try[A]] =
new TrySemigroup[A](Semigroup[A])
implicit def catsKernelCommutativeMonoidForOption[A: CommutativeSemigroup]: CommutativeMonoid[Option[A]] =
cats.kernel.instances.option.catsKernelStdCommutativeMonoidForOption[A]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it this was simply missed in @travisbrown's conversion of all these instances? If so, then I'm 👍 on this addition.

@travisbrown travisbrown self-requested a review June 10, 2020 15:48
travisbrown
travisbrown previously approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to remember something being up here, but not any details, and I might be imagining that. In any case this looks reasonable to me now, thanks for fixing it.

@barambani
Copy link
Contributor Author

barambani commented Jun 10, 2020 •
edited
Loading

Looking at it again I'm wondering if it'd be better in CommutativeSemigroupInstances instead. It will not be used for Semigroup or Monoid anyway.

@barambani
Copy link
Contributor Author

I trusted your instinct @travisbrown and in fact there was a problem. This should fix it. I also added a check to verify that the instance is not ambiguous when the type has both Semigroup and CommutativeSemigroup, but also that things like Monoid[Option[A]] and Semigroup[Option[A]] keep working when there's only Semigroup for A.
Thanks. Sorry I dismissed your approval.

@travisbrown travisbrown self-requested a review June 11, 2020 06:09
travisbrown
travisbrown approved these changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@LukaJCB LukaJCB merged commit 73988a4 into typelevel:master Jun 11, 2020
@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djspiewak djspiewak djspiewak left review comments

@LukaJCB LukaJCB LukaJCB approved these changes

@travisbrown travisbrown travisbrown approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.2.0-M3
Development

Successfully merging this pull request may close these issues.
5 participants
@barambani @codecov-commenter @djspiewak @travisbrown @LukaJCB