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

The combineAll method of MapMonoid always returns an empty map #1346

Closed
denisftw opened this issue Sep 1, 2016 · 1 comment
Closed

The combineAll method of MapMonoid always returns an empty map #1346

denisftw opened this issue Sep 1, 2016 · 1 comment
Assignees
Labels

Comments

@denisftw
Copy link
Contributor

denisftw commented Sep 1, 2016

I was updating my tutorial to Cats 0.7.0 and noticed that combining Maps doesn't work as expected.
The combine method works:

import cats.instances.int._, cats.instances.map._

val totals1 = Monoid[Map[String,Int]].combine(Map("a" -> 12, "b" -> 21), Map("a" -> 10))
println(totals1) // Map(a -> 22, b -> 21)

However, the combineAll method doesn't work:

val scores = List(Map("a" -> 12, "b" -> 21), Map("a" -> 10))
val totals2 = Monoid[Map[String,Int]].combineAll(scores)
println(totals2) // Map()

I looked at the combineAll implementation, and it seems that acc is initialized as an empty Map, but it isn't changed throughout the function. Maybe that's the cause of the problem.

@non
Copy link
Contributor

non commented Sep 1, 2016

Ugh! Thanks for catching that. Will fix (and add tests to catch this).

non pushed a commit to non/cats that referenced this issue Sep 1, 2016
These tests will catch sitautions where the optimized methods are
broken (i.e. become inconsistent with the behavior of .combine).
These tests caught an issue with Monoid[Map[K, V]].combineAll, which
was broken.

Fixes typelevel#1346.
@non non self-assigned this Sep 1, 2016
@non non closed this as completed in #1347 Sep 1, 2016
@non non removed the in progress label Sep 1, 2016
@non non mentioned this issue Sep 2, 2016
non pushed a commit that referenced this issue Sep 2, 2016
These tests will catch sitautions where the optimized methods are
broken (i.e. become inconsistent with the behavior of .combine).
These tests caught an issue with Monoid[Map[K, V]].combineAll, which
was broken.

Fixes #1346.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants