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 more foldMapK shortcircuiting #3447

Closed
LukaJCB opened this issue May 31, 2020 · 5 comments · Fixed by #3451
Closed

Add more foldMapK shortcircuiting #3447

LukaJCB opened this issue May 31, 2020 · 5 comments · Fixed by #3451

Comments

@LukaJCB
Copy link
Member

LukaJCB commented May 31, 2020

We added short-circuting for Vector and List in #3446, but it's still missing a few others, namely:

  • Stream/LazyList
  • NonEmptyList
  • NonEmptyLazyList/Stream
  • SortedMap
  • Queue
  • Chain
  • NonEmptyChain
  • NonEmptyVector
  • SortedSet
  • NonEmptySet
@barambani
Copy link
Contributor

I can look into this if that's ok.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 1, 2020

@barambani please by all means do 😊

@barambani
Copy link
Contributor

barambani commented Jun 1, 2020

I have a couple of questions about this. Now that there's a combineKEval I was wondering if changing the foldMapK in Foldable to

def foldMapK[G[_], A, B](fa: F[A])(f: A => G[B])(implicit G: MonoidK[G]): G[B] =
    foldRight(fa, Eval.now(G.empty[B])) { (a, evalGb) =>
      G.combineKEval(f(a), evalGb)
    }.value

instead of specializing it in the instances would break any assumption (the tests and Mima checks pass). It would delegate the short circuiting behavior to the MonoidK for any implementation, so it looks more general.
Also, the way the ShortCircuitingLaws and relative tests are currently seems to differ from the organization of cats.laws. It seems they could be part of the Foldable, Traverse and TraverseFilter laws/tests. Any reason why they are in a separate file ?

More than happy to proceed in any way. Thanks.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 1, 2020

@barambani great catch, I think that should be fine if it passes the ShortCircuitingTests.
I'm not exactly sure what the reasoning is for it not being part of the more general laws, but it's possible that some people don't care about the short-circuiting and just want to have the "standard" laws checked.
We could however make the foldable laws the parent of the traverse laws in ShortCircuitingTests. Hope that answers your questions.

@barambani
Copy link
Contributor

Makes sense. Thanks a lot :).

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

Successfully merging a pull request may close this issue.

2 participants