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

Infer dependent parameter in NonEmpty/ParallelTests/Laws #3046

Merged

Conversation

djspiewak
Copy link
Member

@djspiewak djspiewak commented Sep 9, 2019

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 with OneAnd and a scalac bug. Putting up the @milessabin signal!

Also this introduces an interesting pattern, which is adding def Aux alongside type Aux when you have def apply. I would propose we should do this for Parallel itself as well, but I didn't make that change. I think this is better than the current DummyImplicit approach, but I'm okay with going with DummyImplicit instead.

kailuowang
kailuowang previously approved these changes Sep 9, 2019
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

👍

@codecov-io
Copy link

codecov-io commented Sep 9, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
laws/src/main/scala/cats/laws/ParallelLaws.scala 100% <100%> (ø) ⬆️
...rc/main/scala/cats/laws/NonEmptyParallelLaws.scala 100% <100%> (ø) ⬆️
...a/cats/laws/discipline/NonEmptyParallelTests.scala 100% <100%> (ø) ⬆️
...ain/scala/cats/laws/discipline/ParallelTests.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 b625536...8bb92f9. Read the comment docs.

@kailuowang kailuowang merged commit 6f721df into typelevel:master Sep 9, 2019
@kailuowang kailuowang added this to the 2.0.0 milestone Sep 9, 2019
@djspiewak
Copy link
Member Author

Random clarification: this is only source-breaking if someone wrote their own laws derived from ParallelLaws. Anything else should be source-compatible.

Copy link
Contributor

@travisbrown travisbrown left a 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] =
Copy link
Contributor

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.

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.

5 participants