conversion `PartialOrder` to `PartialOrdering` and `Hash` to `Hashing` #2116

kailuowang · 2017-12-17T04:06:18Z

To address some items in #2093
Note that this code change is binary compatible in kernel (I tested Mima without any of the exceptions I added) but not binary compatible in core.

…hing

kailuowang · 2017-12-17T04:11:24Z

@denisrosset , will you take a look as well?

codecov-io · 2017-12-17T05:49:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2116      +/-   ##
==========================================
+ Coverage   94.62%   94.63%   +<.01%     
==========================================
  Files         325      325              
  Lines        5511     5514       +3     
  Branches      207      213       +6     
==========================================
+ Hits         5215     5218       +3     
  Misses        296      296

Impacted Files	Coverage Δ
core/src/main/scala/cats/instances/hash.scala	`100% <ø> (ø)`	⬆️
kernel/src/main/scala/cats/kernel/Order.scala	`88.88% <ø> (-0.31%)`	⬇️
...e/src/main/scala/cats/instances/partialOrder.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/instances/eq.scala	`80% <ø> (ø)`	⬆️
core/src/main/scala/cats/instances/order.scala	`83.33% <ø> (ø)`	⬆️
...rnel/src/main/scala/cats/kernel/PartialOrder.scala	`91.42% <100%> (+0.51%)`	⬆️
kernel/src/main/scala/cats/kernel/Eq.scala	`93.75% <100%> (ø)`	⬆️
kernel/src/main/scala/cats/kernel/Hash.scala	`91.66% <100%> (+1.66%)`	⬆️

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 e8fb7ed...262418f. Read the comment docs.

LukaJCB

Thanks!

ceedubs

LGTM but I'd like to get input from @johnynek and/or @denisrosset

denisrosset · 2017-12-18T00:07:27Z

Why do both the core instances and the PartialOrder companion object extends the trait containing implicit conversion?

denisrosset · 2017-12-18T00:12:28Z

kernel/src/main/scala/cats/kernel/PartialOrder.scala

+  def fromPartialOrdering[A](implicit ev: PartialOrdering[A]): PartialOrder[A] = from[A] { (a, b) =>
+    ev.tryCompare(a, b).fold(Double.NaN)(_.toDouble)
+  }
+}


I'd prefer extending the PartialOrder trait here instead of having so many lambdas (from and the fold).

kailuowang · 2017-12-18T01:08:48Z

@denisrosset the core instances extends it so that it's available through import cats.implicits._, the PartialOrder companion object extends it to maintain compatibility.

denisrosset · 2017-12-18T01:14:09Z

So, if I understand correctly, the conversion will always be available regardless of the import cats.implicits._ directive?

kailuowang · 2017-12-18T01:31:09Z

no, it will only be available through import cats.implicits._. The one in the companion object is of little use.

Update: To be clear, the original way it was added was basically wrong - it's added as an implicit but in a companion that won't get searched.

denisrosset · 2017-12-18T11:34:12Z

Update: To be clear, the original way it was added was basically wrong - it's added as an implicit but in a companion that won't get searched.

I suggest to document this, implicits are confusing enough. If you don't want to modify this PR, I'll add a comment on the Google doc and will have a pass a it later.

denisrosset · 2017-12-18T12:11:03Z

A couple of comments.

Eq does not have a conversion to Equiv.
Hash has an implicit conversion to Hashing, but a contradictory comment in the Hash typeclass itself.
No toHashing, toPartialOrdering, toEquiv methods.
The implicit conversion for Order is on instances.order, but the one for PartialOrder on instances.partialOrdering.

This is corrected by the commit:

denisrosset@2b0a67f

LukaJCB · 2017-12-18T12:31:59Z

Awesome @denisrosset! Would you like to PR that to Kai's branch? :)

kailuowang · 2017-12-18T12:51:17Z

@denisrosset I will address your feedbacks.
In regards to the toHashing, toPartialOrdering, toEquiv methods though, they will break binary compat for those type classes. If it's okay to break kernel.implicits I can add the implicits conversion traits there so that kernel users can have them through import cats.kernel.implicits._. WDYT?

LukaJCB · 2017-12-18T13:10:19Z

As far as I can tell from the source, neither Algebra, nor Spire or Algebird make use of kernel.implicits.

kailuowang · 2017-12-18T14:39:23Z

Hash#toHashing deliberately not implemented since scala.util.hashing.Hashing is only
compatible with universal equality.

This is probably a good reason for not providing any Hash to Hashing conversion, no?

kailuowang · 2017-12-18T15:29:09Z

@denisrosset I've added the conversions to cats.kernel.instances.all so that they can be available for kernel users through reports. I removed the Hash -> Hashing conversion due to reason commented above.

denisrosset · 2017-12-18T15:31:14Z

This is probably a good reason for not providing any Hash to Hashing conversion, no?

Except that the documentation for Hashing in the 2.12 api is:

Note: when using a custom Hashing, make sure to use it with the Equiv such that if any two objects are equal, then their hash codes must be equal.

kailuowang · 2017-12-18T15:38:43Z

@denisrosset is it the same way in 2.11 and 2.10?

denisrosset · 2017-12-18T15:42:47Z

See https://static.javadoc.io/org.scala-lang/scala-library/2.10.0/scala/util/hashing/Hashing.html

kailuowang · 2017-12-18T16:15:29Z

@denisrosset that seems right. I couldn't find from the original PR why that comment was added.
Added the Hashing conversion back.

added conversion from PartialOrder to PartialOrdering and Hash to Has…

6072552

…hing

kailuowang requested review from johnynek and ceedubs December 17, 2017 04:06

kailuowang added the in progress label Dec 17, 2017

added fromPartialOrdering

7c0b90d

kailuowang added enhancement bug labels Dec 17, 2017

kailuowang added this to the 1.0.0-RC2 milestone Dec 17, 2017

kailuowang mentioned this pull request Dec 17, 2017

Code review: cats-kernel #2093

Closed

LukaJCB previously approved these changes Dec 17, 2017

View reviewed changes

ceedubs previously approved these changes Dec 17, 2017

View reviewed changes

denisrosset reviewed Dec 18, 2017

View reviewed changes

address feedback

2f836a7

kailuowang dismissed stale reviews from ceedubs and LukaJCB via 2f836a7 December 18, 2017 01:06

kailuowang closed this Dec 18, 2017

kailuowang removed the in progress label Dec 18, 2017

kailuowang reopened this Dec 18, 2017

kailuowang added the in progress label Dec 18, 2017

address feedback

b32a7c7

added hash conversion

262418f

kailuowang force-pushed the partialOrder branch from 0f34e7c to 262418f Compare December 18, 2017 16:17

LukaJCB approved these changes Dec 18, 2017

View reviewed changes

LukaJCB requested a review from ceedubs December 18, 2017 19:13

LukaJCB merged commit 02b819c into typelevel:master Dec 18, 2017

stew removed the in progress label Dec 18, 2017

kailuowang deleted the partialOrder branch December 18, 2017 21:25

ctongfei mentioned this pull request Dec 19, 2017

Remove conversion from Hash to Hashing #2120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conversion `PartialOrder` to `PartialOrdering` and `Hash` to `Hashing` #2116

conversion `PartialOrder` to `PartialOrdering` and `Hash` to `Hashing` #2116

kailuowang commented Dec 17, 2017

kailuowang commented Dec 17, 2017

codecov-io commented Dec 17, 2017 •

edited

Loading

LukaJCB left a comment

ceedubs left a comment

denisrosset commented Dec 18, 2017 •

edited

Loading

denisrosset Dec 18, 2017

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017 •

edited

Loading

denisrosset commented Dec 18, 2017

denisrosset commented Dec 18, 2017 •

edited

Loading

LukaJCB commented Dec 18, 2017

kailuowang commented Dec 18, 2017

LukaJCB commented Dec 18, 2017

kailuowang commented Dec 18, 2017

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017

conversion PartialOrder to PartialOrdering and Hash to Hashing #2116

conversion PartialOrder to PartialOrdering and Hash to Hashing #2116

Conversation

kailuowang commented Dec 17, 2017

kailuowang commented Dec 17, 2017

codecov-io commented Dec 17, 2017 • edited Loading

Codecov Report

LukaJCB left a comment

Choose a reason for hiding this comment

ceedubs left a comment

Choose a reason for hiding this comment

denisrosset commented Dec 18, 2017 • edited Loading

denisrosset Dec 18, 2017

Choose a reason for hiding this comment

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017 • edited Loading

denisrosset commented Dec 18, 2017

denisrosset commented Dec 18, 2017 • edited Loading

LukaJCB commented Dec 18, 2017

kailuowang commented Dec 18, 2017

LukaJCB commented Dec 18, 2017

kailuowang commented Dec 18, 2017

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017

denisrosset commented Dec 18, 2017

kailuowang commented Dec 18, 2017

conversion `PartialOrder` to `PartialOrdering` and `Hash` to `Hashing` #2116

conversion `PartialOrder` to `PartialOrdering` and `Hash` to `Hashing` #2116

codecov-io commented Dec 17, 2017 •

edited

Loading

denisrosset commented Dec 18, 2017 •

edited

Loading

kailuowang commented Dec 18, 2017 •

edited

Loading

denisrosset commented Dec 18, 2017 •

edited

Loading