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

Rename Inject to InjectK #1596

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Conversation

andyscott
Copy link
Contributor

@andyscott andyscott commented Apr 8, 2017

Rename Inject to InjectK to be consistent with names/renames for EitherK, FunctionK, Tuple2K, and friends. See #1589.

This is a breaking API and breaking binary change.

Additionally, this opens the door to "re-introduce" Inject for Either:

sealed abstract class Inject[A, B] {
  def inj: A => B
  def prj: B => Option[A]
  def apply(a: A): B = inj(a)
  def unapply(b: B): Option[A] = prj(b)
}

private[cats] sealed abstract class InjectInstances {
  implicit def catsReflexiveInjectInstance[A]: Inject[A, A] =
    new Inject[A, A] {
      val inj = identity
      val prj = Some(_)
    }

  implicit def catsLeftInjectInstance[A, B]: Inject[A, Either[A, B]] =
    new Inject[A, Either[A, B]] {
      val inj = Left(_)
      val prj = _.left.toOption
    }

  implicit def catsRightInjectInstance[A, B, C](implicit I: Inject[A, B]): Inject[A, Either[C, B]] =
    new Inject[A, Either[C, B]] {
      val inj = a => Right(I.inj(a))
      val prj = _.right.toOption.flatMap(I.prj)
    }

}

object Inject extends InjectInstances {
  def apply[A, B](implicit I: Inject[A, B]): Inject[A, B] = I
}

Inject for Either would be useful when combining error algebras across domains with Free.

@codecov-io
Copy link

codecov-io commented Apr 8, 2017

Codecov Report

Merging #1596 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1596   +/-   ##
=======================================
  Coverage   93.11%   93.11%           
=======================================
  Files         250      250           
  Lines        3992     3992           
  Branches      138      136    -2     
=======================================
  Hits         3717     3717           
  Misses        275      275
Impacted Files Coverage Δ
core/src/main/scala/cats/package.scala 100% <ø> (ø) ⬆️
free/src/main/scala/cats/free/Free.scala 88.23% <100%> (ø) ⬆️
core/src/main/scala/cats/InjectK.scala 100% <100%> (ø)

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 4e381d2...e5ec454. Read the comment docs.

@kailuowang
Copy link
Contributor

Makes sense to me. 👍 Can we also create a ticket for the new Inject? It would be a bit awkward to have InjectK but not Inject.

@kailuowang kailuowang added this to the cats-1.0.0 milestone Apr 9, 2017
@andyscott
Copy link
Contributor Author

Sure, and I'll put up a PR up for Inject if this one gets merged.

This was referenced Apr 9, 2017
@ceedubs
Copy link
Contributor

ceedubs commented Apr 17, 2017

Thanks @andyscott this looks good to me. I only have one minor suggestion: there are a couple places that now say "InjectK type class as described in "Data types a la carte". This might be slightly confusing to readers, since that paper doesn't mention an InjectK. Maybe we could just change this text to "The injection type class as described..." or something?

@andyscott
Copy link
Contributor Author

@ceedubs Good call, I'll address that now

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 @andyscott! 👍 LGTM.

@kailuowang kailuowang merged commit 50741da into typelevel:master Apr 21, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants