-
-
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 Compose instance for Map #2402
Conversation
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.
That was super quick, thanks much! :)
Fix binary compatibility issues by moving the new Compose instance for Map to a separate trait.
Codecov Report
@@ Coverage Diff @@
## master #2402 +/- ##
==========================================
- Coverage 95.01% 94.88% -0.14%
==========================================
Files 349 350 +1
Lines 6000 6258 +258
Branches 222 284 +62
==========================================
+ Hits 5701 5938 +237
- Misses 299 320 +21
Continue to review full report at Codecov.
|
@@ -15,6 +15,8 @@ class MapSuite extends CatsSuite { | |||
checkAll("Map[Int, Int] with Option", UnorderedTraverseTests[Map[Int, ?]].unorderedTraverse[Int, Int, Int, Option, Option]) | |||
checkAll("UnorderedTraverse[Map[Int, ?]]", SerializableTests.serializable(UnorderedTraverse[Map[Int, ?]])) | |||
|
|||
checkAll("Compose[Map]", ComposeTests[Map].compose[Int, Long, String, Double]) |
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.
nit, can we add the SerializableTests
as the other instances here?
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 @denisrosca! That was a quick turnaround and an excellent job maintaining binary compatibility.
Looking at your implementation, it looks correct. However, I feel a bit uneasy about relying solely on the ComposeLaws
as tests. All they test is associativity, so I believe that a trivial implementation that always returned an empty Map
would satisfy the CompseLaws
. Could you please add some tests verifying that Map
composition works as one would expect? I'd even be satisfied with just adding some sbt-doctest examples.
|
||
trait MapInstancesBinCompat0 { | ||
|
||
implicit def catsStdComposeForMap: Compose[Map] = new Compose[Map] { |
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 this be a val
instead of a def
so the same instance can be reused repeatedly?
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.
done
@ceedubs Would a doctest like this be enough?
|
@denisrosca that example looks great! |
* Add Compose instance for Map * Move Compose instance for Map to separate trait Fix binary compatibility issues by moving the new Compose instance for Map to a separate trait. * Serialization test for Compose instance for Map * Add doctest for Compose[Map]
Closes #2399