Skip to content

Add Compose instance for Map#2402

Merged
LukaJCB merged 4 commits into
typelevel:masterfrom
denisrosca:master
Aug 16, 2018
Merged

Add Compose instance for Map#2402
LukaJCB merged 4 commits into
typelevel:masterfrom
denisrosca:master

Conversation

@denisrosca
Copy link
Copy Markdown
Contributor

@denisrosca denisrosca commented Aug 14, 2018

Closes #2399

LukaJCB
LukaJCB previously approved these changes Aug 14, 2018
View reviewed changes
Copy link
Copy Markdown
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! :)

@denisrosca
Move Compose instance for Map to separate trait
c6abd50 
Fix binary compatibility issues by moving the new Compose instance for Map
to a separate trait.
@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 14, 2018 •
edited
Loading

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
View reviewed changes
kailuowang
kailuowang reviewed Aug 14, 2018
View reviewed changes
Comment thread tests/src/test/scala/cats/tests/MapSuite.scala Outdated
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
Copy Markdown
Contributor

@kailuowang kailuowang Aug 14, 2018

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
View reviewed changes
ceedubs
ceedubs suggested changes Aug 15, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread core/src/main/scala/cats/instances/map.scala Outdated

trait MapInstancesBinCompat0 {

implicit def catsStdComposeForMap: Compose[Map] = new Compose[Map] {
Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Aug 15, 2018

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
Copy Markdown
Contributor Author

@denisrosca denisrosca Aug 15, 2018

Choose a reason for hiding this comment

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

done

@denisrosca
Copy link
Copy Markdown
Contributor Author

denisrosca commented Aug 15, 2018

@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
Copy Markdown
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
@denisrosca @catostrophe
Add Compose instance for Map (typelevel#2402)
dce315d 
* 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

Reviewers

@LukaJCB LukaJCB LukaJCB approved these changes

+2 more reviewers

@kailuowang kailuowang kailuowang left review comments

@ceedubs ceedubs ceedubs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.3

Development

Successfully merging this pull request may close these issues.

5 participants

@denisrosca @codecov-io @ceedubs @kailuowang @LukaJCB