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

Get Semigroupal from Arrow for free #2069

Closed
kailuowang opened this issue Dec 6, 2017 · 7 comments
Closed

Get Semigroupal from Arrow for free #2069

kailuowang opened this issue Dec 6, 2017 · 7 comments

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Dec 6, 2017

We could add the following to Semigroupal's companion

  implicit def catsSemigroupalForArrow[F[_, _], A](implicit F: Arrow[F]): Semigroupal[F[A, ?]] = new Semigroupal[F[A, ?]] {
    def product[B, C](fb: F[A, B], fc: F[A, C]): F[A, (B, C)] =
      F.andThen(F.lift((x: A) => (x, x)), F.split(fb, fc))
  }

#2063 is adding a merge method, so this instance can just delegate to that.
To test this we might need to create a new case class MyFunc[A, B](f: A => B) and an Arrow instance for it and then test the Semigroupal laws against it.

I set the milestone to be 1.1, but it can certainly be done by 1.0

@LukaJCB
Copy link
Member

LukaJCB commented Dec 6, 2017

Would this be a binary compatible change?

@kailuowang
Copy link
Contributor Author

I think so.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 6, 2017

A relevant question would be whether there are conceivable F types that form an Arrow but that we wouldn't want to derive this implicit Semigroupal for. I can't think of anything that might fall into that realm, but it's probably worth thinking about.

@kailuowang
Copy link
Contributor Author

@ceedubs are you suggesting making it explicit?

@ceedubs
Copy link
Contributor

ceedubs commented Dec 6, 2017

@kailuowang that was one thought that I had. But I don't know if there's really any reason to that. Just my usual paranoia :). I'll ponder this a bit and give others a chance to weigh in over the next day or two, and if nobody can think of a reason that this implicit def shouldn't exist, then maybe it's either good or mostly harmless.

@stephen-lazaro
Copy link
Contributor

stephen-lazaro commented Dec 7, 2017

I can open a PR for this. I think you can reasonably strengthen it to Apply as well, after playing with it a bit (sorry Kailuo, I mislead you the other day :) ). Given that that's a fair amount stronger, it might lend more weight to explicitness over implicitness here.

@LukaJCB
Copy link
Member

LukaJCB commented Dec 15, 2017

Fixed by #2078

@LukaJCB LukaJCB closed this as completed Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants