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

CommutativeMonoid[Option[A]] from CommutativeSemigroup #2834

Merged
merged 14 commits into from
Nov 6, 2019
Merged

CommutativeMonoid[Option[A]] from CommutativeSemigroup #2834

merged 14 commits into from
Nov 6, 2019

Conversation

valydia
Copy link
Contributor

@valydia valydia commented May 8, 2019 •
edited
Loading

As mentioned by @kailuowang, this change will be binary breaking on scala 2.11, it is schedule for 2.1

@valydia valydia force-pushed the commutative-monoid-for-option branch from 4ebc19c to 5e555d6 Compare May 8, 2019 13:11
@kailuowang
Copy link
Contributor

Thanks, shall we also change these tests to CommutativeMonoid tests?
https://github.com/typelevel/cats/blob/master/kernel-laws/shared/src/test/scala/cats/kernel/laws/LawTests.scala#L181-L182

valydia
valydia commented May 8, 2019
View reviewed changes
kernel-laws/shared/src/test/scala/cats/kernel/laws/LawTests.scala
checkAll("Monoid[Option[String]]", MonoidTests[Option[String]].monoid)
checkAll("Monoid[Option[String]]", SerializableTests.serializable(Monoid[String]))
checkAll("Monoid[Option[String]]", SerializableTests.serializable(Monoid[Option[String]]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looked like a copy and paste went wrong

@valydia
Copy link
Contributor Author

valydia commented May 8, 2019

Yes, done there cfd5742

@valydia valydia force-pushed the commutative-monoid-for-option branch from cfd5742 to bd00510 Compare May 28, 2019 09:44
@kailuowang kailuowang added this to the 2.1-RC1 milestone Jun 25, 2019
kailuowang
kailuowang previously approved these changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Scheduled to 2.1, so let's wait until 2.0 release to merge this one.

LukaJCB
LukaJCB previously approved these changes Jul 3, 2019
View reviewed changes
@LukaJCB
Copy link
Member

LukaJCB commented Oct 16, 2019

Hi @valydia, can you rebase this with master? :)

@valydia valydia dismissed stale reviews from LukaJCB and kailuowang via 3f6a62d October 17, 2019 08:36
@codecov-io
Copy link

codecov-io commented Oct 17, 2019 •
edited
Loading

Codecov Report

Merging #2834 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2834      +/-   ##
==========================================
+ Coverage   93.17%   93.19%   +0.02%     
==========================================
  Files         372      376       +4     
  Lines        7179     7323     +144     
  Branches      207      190      -17     
==========================================
+ Hits         6689     6825     +136     
- Misses        490      498       +8
Flag Coverage Δ
#scala_version_212 93.53% <100%> (+0.02%) ⬆️
#scala_version_213 90.86% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
.../scala/cats/kernel/instances/OptionInstances.scala 100% <100%> (ø) ⬆️
...ala/cats/kernel/instances/SortedSetInstances.scala 91.66% <0%> (-8.34%) ⬇️
core/src/main/scala/cats/instances/sortedSet.scala 85.71% <0%> (-2.53%) ⬇️
core/src/main/scala/cats/instances/option.scala 98.14% <0%> (-1.86%) ⬇️
core/src/main/scala/cats/data/Chain.scala 98.43% <0%> (-1.24%) ⬇️
core/src/main/scala/cats/instances/queue.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/ApplicativeError.scala 100% <0%> (ø) ⬆️
...n/scala/cats/kernel/instances/QueueInstances.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <0%> (ø) ⬆️
...in/scala/cats/kernel/instances/ListInstances.scala 100% <0%> (ø) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7ff8f...bdee733. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Oct 17, 2019
View reviewed changes
travisbrown
travisbrown previously approved these changes Nov 5, 2019
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.

Looks good to me, apart from possibly making the new helper class private.

kernel/src/main/scala/cats/kernel/instances/OptionInstances.scala Outdated
@@ -80,3 +82,7 @@ class OptionMonoid[A](implicit A: Semigroup[A]) extends Monoid[Option[A]] {
}
}
}

class OptionCommutativeMonoid[A](implicit A: CommutativeSemigroup[A])
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about making this private? I think introducing all these helper classes into the public API was a mistake, and we can at least avoid doing more damage moving forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

@valydia Do you mind if I push this change and we can get this merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, go ahead! Sorry I've been busy lately! Thanks

@travisbrown
Copy link
Contributor

@LukaJCB Think we can go ahead and merge this when green?

@LukaJCB
Copy link
Member

LukaJCB commented Nov 6, 2019

Sounds good to me 👍

@kailuowang kailuowang merged commit 1a6d2c3 into typelevel:master Nov 6, 2019
@valydia
Copy link
Contributor Author

valydia commented Nov 6, 2019

Thanks for the assist @travisbrown 👌

@valydia valydia deleted the commutative-monoid-for-option branch November 8, 2019 13:10
This was referenced Nov 13, 2019
Closed
Merged
@kubukoz
Copy link
Member

kubukoz commented Dec 21, 2019

Can #2725 be closed now?

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Feb 29, 2020
@gagandeepkalra
backported typelevel#2834 added CommutativeMonoid[Option[A]] instance…
069f0b7 
…, in binary compatible way
travisbrown pushed a commit that referenced this pull request Mar 11, 2020
@gagandeepkalra
backported #2834 added CommutativeMonoid[Option[A]] instance, in bina…
3d5a5c2 
…ry compatible way (#3334)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisbrown travisbrown travisbrown left review comments

@kailuowang kailuowang kailuowang approved these changes

@LukaJCB LukaJCB LukaJCB left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.1.0-RC1
Development

Successfully merging this pull request may close these issues.
6 participants
@valydia @kailuowang @LukaJCB @codecov-io @travisbrown @kubukoz