Remove unused parameters from methods #1867
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1867 +/- ##
==========================================
- Coverage 94.96% 94.92% -0.05%
==========================================
Files 241 241
Lines 4193 4193
Branches 109 117 +8
==========================================
- Hits 3982 3980 -2
- Misses 211 213 +2
Continue to review full report at Codecov.
|
|
The failing coverage test is : private[this] val singleton: NotNull[Any] = new NotNull[Any] {}
implicit def catsNotNullForA[A]: NotNull[A] = singleton.asInstanceOf[NotNull[A]]So, iff it's OK to change them in the API in this PR (and I suspect not, actually), the next question is whether to remove these two lines or add tests for them. If they should stay, perhaps I should comment accordingly? |
There was a problem hiding this comment.
Thanks @BennyHill! I left a comment about why I don't think that we can remove the NotNull parameters, but I suspect the other change is a good one, and I'm glad that you pointed out that the NotNull usage is not very well documented.
| @@ -24,7 +24,7 @@ object EitherSyntax { | |||
| * Uses the [[http://typelevel.org/cats/guidelines.html#partially-applied-type-params Partially Applied Type Params technique]] for ergonomics. | |||
| */ | |||
| private[syntax] final class CatchOnlyPartiallyApplied[T](val dummy: Boolean = true ) extends AnyVal { | |||
| def apply[A](f: => A)(implicit CT: ClassTag[T], NT: NotNull[T]): Either[T, A] = | |||
| def apply[A](f: => A)(implicit CT: ClassTag[T]): Either[T, A] = | |||
There was a problem hiding this comment.
While the NT parameter isn't referenced at runtime, it serves an important compile-time role. Consider that you wanted to do something like the following:
scala> Either.catchOnly[NumberFormatException]("foo".toInt)
res2: Either[NumberFormatException,Int] = Left(java.lang.NumberFormatException: For input string: "foo")If you forget to add the type parameter before this PR you get a compile-time error:
scala> Either.catchOnly("foo".toInt)
<console>:18: error: ambiguous implicit values:
both method If you are seeing this, you probably need to add an explicit type parameter somewhere, because Null is being inferred. in object NotNull of type => cats.NotNull[Null]
and method catsAmbiguousNotNullNull2 in object NotNull of type => cats.NotNull[Null]
match expected type cats.NotNull[Null]
Either.catchOnly("foo".toInt)However, with this PR you get a runtime error because Null is inferred as the type parameter:
scala> Either.catchOnly("foo".toInt)
java.lang.NumberFormatException: For input string: "foo"
at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
at java.lang.Integer.parseInt(Integer.java:580)
at java.lang.Integer.parseInt(Integer.java:615)
at scala.collection.immutable.StringLike.toInt(StringLike.scala:301)
at scala.collection.immutable.StringLike.toInt$(StringLike.scala:301)
at scala.collection.immutable.StringOps.toInt(StringOps.scala:29)
at .$anonfun$res0$1(<console>:15)
at scala.runtime.java8.JFunction0$mcI$sp.apply(JFunction0$mcI$sp.java:12)
at cats.syntax.EitherSyntax$CatchOnlyPartiallyApplied$.apply$extension(either.scala:29)
... 39 elidedI think that the correct change to make here is to document why this NotNull parameter exists. The same goes for the Validated case.
| @@ -8,7 +8,7 @@ trait PartialOrderSyntax extends EqSyntax { | |||
| new PartialOrderOps[A](a) | |||
| } | |||
|
|
|||
| final class PartialOrderOps[A](lhs: A)(implicit A: PartialOrder[A]) { | |||
| final class PartialOrderOps[A](lhs: A) { | |||
There was a problem hiding this comment.
I have no idea about the macro expansion in machinist.
I am not sure that is what is going on here anyway. I think it may be to prevent this class from being allocated without a PartialOrder.
We could I'm not sure if this is even tested sadly.
There was a problem hiding this comment.
This seems no longer relevant, since macros have been now removed in preparation for Dotty.
The main goal of this PR was to add syntax for the `Order` and `PartialOrder` syntax (provided via machinist ops) to address [this comment](typelevel#1867 (comment)). In the process I noticed that the Order companion object didn't have a `comparison` function to forward through to the type class instance like it did for other methods, so I added that. I did notice that the `Order`/`PartialOrder` syntax does work whether or not you take the `Order`/`PartialOrder` instances as implicit parameters to the ops classes. I think that only @non understands quite what's going on with the machinist syntax macros, so I didn't make the changes to take away those implicit parameters without knowing the implications. The tests pass either way, so I think that end-code should work for users either way.
* Add comparison method in Order companion object The main goal of this PR was to add syntax for the `Order` and `PartialOrder` syntax (provided via machinist ops) to address [this comment](#1867 (comment)). In the process I noticed that the Order companion object didn't have a `comparison` function to forward through to the type class instance like it did for other methods, so I added that. I did notice that the `Order`/`PartialOrder` syntax does work whether or not you take the `Order`/`PartialOrder` instances as implicit parameters to the ops classes. I think that only @non understands quite what's going on with the machinist syntax macros, so I didn't make the changes to take away those implicit parameters without knowing the implications. The tests pass either way, so I think that end-code should work for users either way. * Change PartialOrder syntax tests to handle NaN
|
Closing this because the two proposed changes are not desired or now outdated. |
see also #1632 (review)
scala/scala#5402 (comment)
scala/scala#5876
See #1669 for related work on
MonadErrorandApplicativeError