-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Infer dependent parameter in NonEmpty/ParallelTests/Laws #3046
Infer dependent parameter in NonEmpty/ParallelTests/Laws #3046
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Codecov Report
@@ Coverage Diff @@
## master #3046 +/- ##
==========================================
+ Coverage 93.46% 93.46% +<.01%
==========================================
Files 368 368
Lines 6975 6979 +4
Branches 187 184 -3
==========================================
+ Hits 6519 6523 +4
Misses 456 456
Continue to review full report at Codecov.
|
Random clarification: this is only source-breaking if someone wrote their own laws derived from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't make this change in #3012 intentionally, since I thought it would be better to make what's tested more explicit, and since writing out the extra type parameter once isn't really a big usability issue in tests. I meant to flag that decision in the PR but forgot.
Given that this PR has turned up issues downstream I now think it's probably the right thing to do.
def apply[M[_]](implicit ev: NonEmptyParallel[M]): NonEmptyParallelLaws.Aux[M, ev.F] = | ||
apply[M, ev.F](ev, implicitly) | ||
|
||
def apply[M[_], F[_]](implicit ev: NonEmptyParallel.Aux[M, F], D: DummyImplicit): NonEmptyParallelLaws.Aux[M, F] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really matter but I did this in the other direction in #3031, with the DummyImplicit
on the new one.
It's a little wonky. Also I left one
.Aux
usage in the tests which came from the fact that the compiler just wasn't able to infer the dependent version. I didn't look into why, but I suspect it's wonkiness withOneAnd
and a scalac bug. Putting up the @milessabin signal!Also this introduces an interesting pattern, which is adding
def Aux
alongsidetype Aux
when you havedef apply
. I would propose we should do this forParallel
itself as well, but I didn't make that change. I think this is better than the currentDummyImplicit
approach, but I'm okay with going withDummyImplicit
instead.