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

Stack-safe Coyoneda #1602

Merged
merged 1 commit into from
Jun 12, 2017
Merged

Conversation

edmundnoble
Copy link
Contributor

Makes Coyoneda's map stack-safe, by keeping a list of functions and reversing it before applying them.
Breaking change.

/** The transformer function, to be lifted into `F` by `run`. */
val k: Pivot => A
/** The transformer functions, to be lifted into `F` by `run`. */
val k: List[Any => Any]
Copy link
Contributor

Choose a reason for hiding this comment

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

Given its signature should this be exposed? Seems like we should hide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say so, but the user should have the power to concatenate the lists IMO. Perhaps a scarier name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, that doesn't sound useful. Making it private[cats] just in case we change it in future.

@@ -53,14 +57,18 @@ object Coyoneda {
/** `F[A]` converts to `Coyoneda[F,A]` for any `F` */
def lift[F[_], A](fa: F[A]): Coyoneda[F, A] = apply(fa)(identity[A])

/** Like `lift(fa).map(_k)`. */
def apply[F[_], A, B](fa: F[A])(k0: A => B): Aux[F, B, A] =
/** unsafe mang */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a better comment than this ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omg I was removing it you fast person

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #1602 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1602      +/-   ##
==========================================
+ Coverage   93.88%   93.88%   +<.01%     
==========================================
  Files         246      246              
  Lines        4088     4090       +2     
  Branches      154      154              
==========================================
+ Hits         3838     3840       +2     
  Misses        250      250
Impacted Files Coverage Δ
free/src/main/scala/cats/free/Coyoneda.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 1931c93...19c6561. Read the comment docs.

/** Creates a `Coyoneda[F, A]` for any `F`, taking an `F[A]`
* and a list of [[Functor.map]]ped functions to apply later
*/
def unsafeApply[F[_], A, B](fa: F[A])(k0: List[Any => Any]): Aux[F, B, A] =
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it (or anything with Any in it) should be public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was on my way to making this private. Not because of Any, but because it fixes List.


import Coyoneda.{Aux, apply}
def execute(a: Pivot): A =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this one need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question: it should in my opinion be public because a) it's as safe as exposing k previously and b) k was public before, which means users have had time to create use-cases that require using the function inside a Coyo on other values.

@johnynek
Copy link
Contributor

I wonder if we want to just add code like:

sealed trait StackSafeFn[A, B] {
  def apply(a: A): B = StackSafeFn.run(this, a)
}
object StackSafeFn {
  private case class FromScala[A, B](fn: A => B) extends StackSafeFn[A, B]
  private case class Compose[A, B, C](first: StackSafeFn[A, B], second: StackSafeFn[B, C]) extends StackSafeFn[A, C]

  private def run[A, B](fn: StackSafeFn[A, B], a: A): B = ...
}

@edmundnoble
Copy link
Contributor Author

@johnynek What we need is Steque, because it's exactly that data structure (but you need Empty as well, and it also deals with things other than A => B). The only issue in your suggestion is that you're missing a notion of "stack-safe value"; a value accompanied by functions which will be applied to it later. Adding that in makes it equivalent to Free[Id, ?] if we implement Free using Steque.

@kailuowang kailuowang mentioned this pull request May 25, 2017
26 tasks
/** The transformer function, to be lifted into `F` by `run`. */
val k: Pivot => A
/** The transformer functions, to be lifted into `F` by `run`. */
private[cats] val k: List[Any => Any]
Copy link
Contributor

@johnynek johnynek Jun 8, 2017

Choose a reason for hiding this comment

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

what if we called this ks and keep def k: Pivot => A = execute _ so we don't break binary compatibility here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done, got it down as final def k and added a doc.


/** Converts to `Yoneda[F,A]` given that `F` is a functor */
final def toYoneda(implicit F: Functor[F]): Yoneda[F, A] =
new Yoneda[F, A] {
def apply[B](f: A => B): F[B] = F.map(fi)(k andThen f)
def apply[B](f: A => B): F[B] = F.map(fi)(execute _ andThen f)
Copy link
Contributor

Choose a reason for hiding this comment

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

if we keep k we can not change this line.

@johnynek
Copy link
Contributor

👍

@kailuowang
Copy link
Contributor

merging with 3 sign-offs

@kailuowang kailuowang merged commit 1ffaeae into typelevel:master Jun 12, 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.

7 participants