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

Add labels to prop produced from IsEq #2052

Merged
merged 1 commit into from
Nov 29, 2017

Conversation

nigredo-tori
Copy link
Contributor

@nigredo-tori nigredo-tori commented Nov 29, 2017

For failed IsEq produce labels with both values to simplify debugging.

The implementation was borrowed from org.scalacheck.Prop.?= with minimal changes. No changes to *Test traits were made, so the default pretty-printer will be used in most cases, which shouldn't cause any problems. Changes to pass a pretty-printer through would be trivial, but would require a lot of small changes (and would be fragile unless we convert IsEq to Prop explicitly everywhere, since we always have implicit pretty-printer for Any).

ev.eqv(isEq.lhs, isEq.rhs)
implicit def catsLawsIsEqToProp[A](isEq: IsEq[A])(implicit ev: Eq[A], pp: A => Pretty): Prop =
isEq match { case IsEq(x, y) =>
if(ev.eqv(x, y)) Prop.proved else Prop.falsified :| {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails the scalastyle check with

no space after token if

@kailuowang
Copy link
Contributor

This is super cool! 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.

Awesome thanks!

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #2052 into master will decrease coverage by 0.05%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2052      +/-   ##
==========================================
- Coverage      95%   94.95%   -0.06%     
==========================================
  Files         311      311              
  Lines        5266     5269       +3     
  Branches      131      129       -2     
==========================================
  Hits         5003     5003              
- Misses        263      266       +3
Impacted Files Coverage Δ
.../src/main/scala/cats/laws/discipline/package.scala 100% <ø> (ø) ⬆️
...in/scala/cats/kernel/laws/discipline/package.scala 25% <25%> (-75%) ⬇️

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 7f403ef...b67f033. Read the comment docs.

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Nice, I think that this is really useful.

@kailuowang kailuowang merged commit 8fa89fb into typelevel:master Nov 29, 2017
@kailuowang kailuowang added this to the 1.0.0 milestone Nov 29, 2017
@ceedubs
Copy link
Contributor

ceedubs commented Nov 29, 2017

I guess that this resulted in a slight code coverage drop. It would probably be nice to have a test case for respecting Pretty instances, but it's not really a huge deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants