Issue #934 - Make Eval covariant #937
Conversation
Current coverage is
|
| @@ -350,7 +350,7 @@ trait EvalMonoid[A] extends Monoid[Eval[A]] with EvalSemigroup[A] { | |||
| trait EvalGroup[A] extends Group[Eval[A]] with EvalMonoid[A] { | |||
| implicit def algebra: Group[A] | |||
| def inverse(lx: Eval[A]): Eval[A] = | |||
| lx.map(_.inverse) | |||
| lx.map(_.inverse()) | |||
There was a problem hiding this comment.
@adelbertc it isn't needed, but the definition of that function in cats.syntax.GroupOps is:
def inverse(): A = macro Ops.unop[A]Not sure why that's the definition, but when you've got a parens pair like that in your def, then you have tools like IntelliJ IDEA complaining about you not using the parens at the call site. Which I personally find cool because those parens usually indicate side-effects. Maybe we should change the def inverse() definition?
There was a problem hiding this comment.
I tried removing the parens from def inverse() and got this:
[error] /Users/cody/code/cats/core/src/main/scala/cats/syntax/group.scala:15: macro implementation has incompatible shape:
[error] required: (c: scala.reflect.macros.whitebox.Context): c.Expr[A]
[error] or : (c: scala.reflect.macros.whitebox.Context): c.Tree
[error] found : (c: reflect.macros.Context)(): c.Expr[R]
[error] number of parameter sections differ
[error] def inverse: A = macro Ops.unop[A]
Paging @non, our macro optimization wizard.
There was a problem hiding this comment.
You would also need to remove the parens from the corresponding macro defn.
|
It's been over 2 weeks since #934 (comment) and nobody has spoken up against this. 👍 from me. |
|
I agree with the rationale. Let's do it! 👍 |
The reason is that
Evalshould be a drop-in replacement for by-name parameters. Noticed that changing it wouldn't break anything within Cats.