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.

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

Merged
merged 4 commits into from
Aug 16, 2018
Merged

Add Compose instance for Map #2402

merged 4 commits into from
Aug 16, 2018

Conversation

denisrosca
Copy link
Contributor

Closes #2399

LukaJCB
LukaJCB previously approved these changes Aug 14, 2018
Copy link
Member

@LukaJCB LukaJCB left a 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-io
Copy link

codecov-io commented Aug 14, 2018

Codecov Report

Merging #2402 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <ø> (ø) ⬆️
core/src/main/scala/cats/instances/map.scala 93.54% <100%> (+0.95%) ⬆️
core/src/main/scala/cats/syntax/foldable.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Chain.scala 90.45% <0%> (ø)
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 99.04% <0%> (+0.07%) ⬆️
...main/scala/cats/laws/discipline/ComposeTests.scala 100% <0%> (+20%) ⬆️
laws/src/main/scala/cats/laws/ComposeLaws.scala 100% <0%> (+50%) ⬆️

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 a5141f8...d8ee4bb. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Aug 14, 2018
@@ -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])
Copy link
Contributor

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?

LukaJCB
LukaJCB previously approved these changes Aug 15, 2018
Copy link
Contributor

@ceedubs ceedubs left a 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] {
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@denisrosca
Copy link
Contributor Author

@ceedubs Would a doctest like this be enough?

/**
  * Compose two maps `g` and `f` by using the values in `f` as keys for `g`.
  * {{{
  * scala> import cats.arrow.Compose
  * scala> import cats.implicits._
  * scala> val first = Map(1 -> "a", 2 -> "b", 3 -> "c", 4 -> "a")
  * scala> val second = Map("a" -> true, "b" -> false, "d" -> true)
  * scala> Compose[Map].compose(second, first)
  * res0: Map[Int, Boolean] = Map(1 -> true, 2 -> false, 4 -> true)
  * }}}
  */
  def compose[A, B, C](f: Map[B, C], g: Map[A, B]): Map[A, C]

@ceedubs
Copy link
Contributor

ceedubs commented Aug 15, 2018

@denisrosca that example looks great!

@kailuowang kailuowang added this to the 1.3 milestone Aug 15, 2018
@LukaJCB LukaJCB merged commit 7b3d3b9 into typelevel:master Aug 16, 2018
catostrophe pushed a commit to catostrophe/cats that referenced this pull request Sep 15, 2018
* 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants