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 Eq docs #1788

Merged
merged 7 commits into from
Sep 21, 2017
Merged

Add Eq docs #1788

merged 7 commits into from
Sep 21, 2017

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Aug 5, 2017

Adds some basic documentation for Eq. I'm not sure if this is enough, or if more would complicate things and turn off users, would love to hear some feedback :)

@codecov-io
Copy link

codecov-io commented Aug 5, 2017

Codecov Report

Merging #1788 into master will increase coverage by 0.67%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1788      +/-   ##
==========================================
+ Coverage   94.87%   95.54%   +0.67%     
==========================================
  Files         241      248       +7     
  Lines        4139     4426     +287     
  Branches      103      116      +13     
==========================================
+ Hits         3927     4229     +302     
+ Misses        212      197      -15
Impacted Files Coverage Δ
kernel/src/main/scala/cats/kernel/Order.scala 86.48% <0%> (-2.71%) ⬇️
core/src/main/scala/cats/Monad.scala 96% <0%> (-0.16%) ⬇️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 92.06% <0%> (-0.13%) ⬇️
...in/scala/cats/laws/discipline/ReducibleTests.scala 100% <0%> (ø) ⬆️
...e/src/main/scala/cats/instances/partialOrder.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/Foldable.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/functor/Bifunctor.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/syntax/cartesian.scala 50% <0%> (ø) ⬆️
...c/main/scala/cats/laws/discipline/ApplyTests.scala 100% <0%> (ø) ⬆️
... and 41 more

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 0519906...bfb63e0. Read the comment docs.

"Hello" =!= "World"
```

Implementing `Eq` instances yourself for every data type might seem like huge drawback compared to only slight gains of typesafety.
Copy link
Contributor

Choose a reason for hiding this comment

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

we could mention fromUniversalEquals which should work fine for case classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Contributor

@gabro gabro Aug 9, 2017

Choose a reason for hiding this comment

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

not sure this should go in the documentation, but I recently found this useful:

implicit def productEq[A <: Product]: Eq[A] = Eq.fromUniversalEquals

unless I'm missing something (and if so, please let me know!) fromUniversalEqual should work fine with any Product. This makes it incredibly easy for the users to provide a reasonable Eq instance for every case class.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's pretty interesting, seems like it would work for all case classes! Any known downsides?

Copy link
Contributor

Choose a reason for hiding this comment

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

not that I know. We're using it in a project since a few weeks and so far so good.

That said, I'm far from being an expert :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's worth mentioning in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added :)

@kailuowang kailuowang modified the milestone: 1.0.0-RC1 Aug 8, 2017
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!

@LukaJCB LukaJCB mentioned this pull request Aug 8, 2017
70 tasks
Foo(10, "") === Foo(10, "")
```

You can even go one step further and make use of the fact, that every case class will extend `scala.Product`, by creating an `Eq` instance for it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this mechanism is probably good enough for all Product, so it doesn't limit to case classes. How about:

You can even go one step further by making this implicitly available for all scala.Product (which include case classes).

implicit def eqProduct[A <: Product]: Eq[A] = Eq.fromUniversalEquals


# Eq

Show is an alternative to the standard Java `equals` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Show/Eq ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whooops!

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! 👍

@kailuowang kailuowang modified the milestone: 1.0.0-RC1 Aug 29, 2017

case class Bar(a: Double, b: Int)

implicit def eqProduct[A <: Product]: Eq[A] = Eq.fromUniversalEquals
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to disagree with a couple of other commenters, but personally I wouldn't be too inclined to include this example in the docs. A couple of significant difference between this and kittens derivation is that kittens will only derive the Eq instance when all types within the case class have Eq instances, and it will be consistent with those individual Eq instances, while this relies on universal equality so it won't necessarily.

I like the fromUniversalEquals method and use it on occasion, but I think it's a big leap to add this instance into implicit scope for all Product types. I don't want to be a stick in the mud though, so if others want to go forward with this in the docs, that's okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a really good point. It didn't come to my mind that fromUniversalEquals wont recursively use Eq for fields. Agree we should remove this example usage.

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.

Yet some more wonderful documentation from @LukaJCB!

@ceedubs ceedubs merged commit 5232087 into typelevel:master Sep 21, 2017
@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017
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