move instances into separate trait #1659

yilinwei · 2017-05-11T10:30:16Z

The current instances sit in the object of Contravariant.

This means that you can't get an implicit of a subclass - implicitly[Invariant[Order]] doesn't work because the instance is not in the companion object of Order or Invariant and the methods such as imap won't work either.

I would argue that for the behaviour to be consistent with the other instances, these should be moved into separate traits.

Doing a quick grep for Kernel...Instances

are affected.

codecov-io · 2017-05-11T11:35:28Z

Codecov Report

Merging #1659 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #1659      +/-   ##
==========================================
- Coverage   93.96%   93.95%   -0.01%     
==========================================
  Files         241      246       +5     
  Lines        4091     4087       -4     
  Branches      160      157       -3     
==========================================
- Hits         3844     3840       -4     
  Misses        247      247

Impacted Files	Coverage Δ
core/src/main/scala/cats/functor/Invariant.scala	`71.42% <ø> (-6.35%)`	⬇️
...e/src/main/scala/cats/ContravariantCartesian.scala	`66.66% <ø> (-16.67%)`	⬇️
core/src/main/scala/cats/Cartesian.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/InvariantMonoidal.scala	`0% <ø> (-77.78%)`	⬇️
...re/src/main/scala/cats/functor/Contravariant.scala	`100% <ø> (ø)`	⬆️
...main/scala/cats/laws/discipline/FlatMapTests.scala	`100% <ø> (ø)`	⬆️
...e/src/main/scala/cats/instances/partialOrder.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/eq.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/order.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/semigroup.scala	`75% <75%> (ø)`
... and 8 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 80d56e6...fea3b25. Read the comment docs.

ceedubs · 2017-05-11T21:46:49Z

@yilinwei thanks! I think that you raise a good point about implicitly[Invariant[Order]] not working.

While import cats.implicits._ is the approach that we recommend to people, we also support piecemeal imports such as cats.instances.string._. I wonder if we should provide something similar for these instances? If so I'm not quite sure how one should do that. cats.instances.kernel.order._ or something? It seems kind of awkward. And it makes me wonder if instead of trait EqInstances you should have trait KernelEqInstances or something.

edmundnoble · 2017-05-11T22:06:18Z

I like cats.instances.order and the approach in this PR. We provide instances originating in kernel from instances._ already with mixins (see instances.bitSet); I think that fits well with providing instances for types provided from kernel.

yilinwei · 2017-05-12T10:03:42Z

@ceedubs

I forgot about the piecemeal imports; I think we should definitely include them. I also agree with @edmundnoble that they should sit under instances rather than instances.kernel.

ceedubs · 2017-05-12T13:10:42Z

Thanks for adding the piecemeal imports @yilinwei. This looks good to me.

It's a little bit of a head-scratcher to think about the fact that MonoidInstances and import cats.instances.monoid._ don't actually include any Monoid[A] instances. Instead they include functor instances for monoids :). However, I do think that this setup is consistent.

codecov is legitimately reporting a slight code coverage drop on this PR. While it's pretty trivial, it may be good to hit the new piecemeal imports in unit tests to make sure that they work as intended. I also think that it would be good to have the unit tests check some implicit resolutions for things like the Invariant[Order] that was the motivation for this PR.

yilinwei · 2017-05-13T21:44:41Z

@ceedubs I feel those tests ought to be autogenerated, ideally something along the lines of,

canResolve[F, TC] { import ... }

should reify into implicitly[TC1]; implicitly[TC2] .... for the whole of the hierarchy.

wdyt?

yilinwei · 2017-05-16T17:14:58Z

@ceedubs

I've committed an example of what I mean;

The macro generates an implicitly for each typeclass in the hierarchy with the import and any implicit vals in scope.

There are two problems at the moment though;

I can't quite work out how to do test->test dependencies since I'm not too familiar with the javascript cross build. The macro should definitely sit in the test directory.
More importantly, scalac flags the import as unused when it's definitely being used. I've disabled the flag for now; but I'd really like to be able to turn it back on if anyone has any ideas.

peterneyens · 2017-05-16T17:27:13Z

I like the idea of testing the resolution of all the type class instances in the hierarchy.
It would be nice to replace code like this.

yilinwei · 2017-05-16T22:59:21Z

I've simplified the macro to just do the implictly expansion because scalac was doing funny things with the imports.

The names are implicitlyH which stands for implicitlyHierarchy and implicitlyHK which stands for implicitlyHierarchyKind. I'm not particularly happy with the name because H usually stands for heterogenous and could be confusing.

I still need to deal with project structures, but the scalac flag problem should go away.

@peterneyens I've added an example for the WriterT to show it works correctly.

kailuowang · 2017-05-25T17:30:38Z

My original concern is that we might have incentive to go the other direction, i.e. move these instance into the companion objects for maintainability of binary compatibility. #1683
After some inspection of the instances being moved in this PR. It doesnt seem to me that there is a good chance of changing these traits. So 👍 from me. As for automatically generated tests, can we attack them in a separate PR?

yilinwei · 2017-05-25T21:46:11Z

@kailuowang Sure; I was more hoping for feedback on whether people like this approach before taking it any further.

Regardless I'll separate the changes from the original ones tomorrow with the additional tests suggested by @ceedubs.

djspiewak · 2017-05-26T16:33:33Z

👍 from me. So this is good for merge once @yilinwei is finished with it.

kailuowang · 2017-05-26T19:20:49Z

👍 modular build green

peterneyens · 2017-05-28T08:26:41Z

tests/src/test/scala/cats/tests/EqTests.scala

+class EqTests extends FunSuite {
+  {
+    import cats.implicits._
+    implicitly[Invariant[Eq]]


Just nitpicking, but we don't need implicitly in all these tests as simulacrum generates an apply in the companion object. Should we just write Invariant[Eq], Invariant[Monoid], ...?

kailuowang · 2017-05-28T14:55:09Z

core/src/main/scala/cats/instances/monoid.scala

+      val empty = f(fa.empty)
+      def combine(x: B, y: B): B = f(fa.combine(g(x), g(y)))
+      override def combineAll(bs: TraversableOnce[B]): B =
+        f(fa.combineAll(bs.map(g)))


This line was not covered in the original code, which is a bit surprising to me. We can probably address it in a different PR. Just want to keep a note here.

kailuowang · 2017-05-28T14:55:17Z

core/src/main/scala/cats/instances/semigroup.scala

+    def imap[A, B](fa: Semigroup[A])(f: A => B)(g: B => A): Semigroup[B] = new Semigroup[B] {
+      def combine(x: B, y: B): B = f(fa.combine(g(x), g(y)))
+      override def combineAllOption(bs: TraversableOnce[B]): Option[B] =
+        fa.combineAllOption(bs.map(g)).map(f)


This line was not covered in the original code, which is a bit surprising to me. We can probably address it in a different PR. Just want to keep a note here.

kailuowang · 2017-05-28T14:55:32Z

core/src/main/scala/cats/instances/semigroup.scala

+    def pure[A](a: A): Semigroup[A] = new Semigroup[A] {
+      def combine(x: A, y: A): A = a
+      override def combineAllOption(as: TraversableOnce[A]): Option[A] =
+        if (as.isEmpty) None else Some(a)


This line was not covered in the original code. We can probably address it in a different PR. Just want to keep a note here.

peterneyens · 2017-05-28T19:12:07Z

Merging with three thumbs up. Thanks @yilinwei!

…ypelevel#1659)

* cats.kernel.Hash and related instances (#1690) * Hash laws * all test passed * Hash instances for tuples * introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq. * Add type alias in cats._; Add contravariant instance for `Hash` * HashFunctions extends EqFunctions * Move contravariant instances to separate trait, in accordance with (#1659) * revert name change * move EitherInstances1#EitherEq out * Optimize hash computation on case classes without allocating a new object * fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder * Full compliance with how Scala generates hash codes on case classes; +SetHash * Cartesian[Hash] * ContravariantCartesian[Hash] * ContravariantCartesian[Hash]; Hash.fromHashing * style issues * remove unused import * some test cases * some test cases * +test for Contravariant[Hash] * +test for Contravariant[Hash] * Add NEL/NEV one (#1707) `NonEmptyList.one` is analogous to `_.pure[NonEmptyList]`. Alternative for `NonEmptyList.of(x)` where you don't pay the price of the varargs, which isn't used. * move instances into separate trait (#1659) * move instances into separate trait * adding separate objects for piecemeal imports * Adding implicit resolution tests for piecemeal imports for hierarchy * Removing explicit implicitly and using apply method * cats.kernel.Hash and related instances (#1690) * Hash laws * all test passed * Hash instances for tuples * introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq. * Add type alias in cats._; Add contravariant instance for `Hash` * HashFunctions extends EqFunctions * Move contravariant instances to separate trait, in accordance with (#1659) * revert name change * move EitherInstances1#EitherEq out * Optimize hash computation on case classes without allocating a new object * fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder * Full compliance with how Scala generates hash codes on case classes; +SetHash * Cartesian[Hash] * ContravariantCartesian[Hash] * ContravariantCartesian[Hash]; Hash.fromHashing * style issues * remove unused import * some test cases * some test cases * +test for Contravariant[Hash] * +test for Contravariant[Hash] * cats.kernel.Hash and related instances (#1690) * Hash laws * all test passed * Hash instances for tuples * introduce implicit precedence in KernelBoiler: Order > PartialOrder > Eq; Hash > Eq. * Add type alias in cats._; Add contravariant instance for `Hash` * HashFunctions extends EqFunctions * Move contravariant instances to separate trait, in accordance with (#1659) * revert name change * move EitherInstances1#EitherEq out * Optimize hash computation on case classes without allocating a new object * fixing the problem in CI build: method catsKernelStdOrderForChar()cats.kernel.instances.CharOrder in trait cats.kernel.instances.CharInstances has a different result type in current version, where it is cats.kernel.Order rather than cats.kernel.instances.CharOrder * Full compliance with how Scala generates hash codes on case classes; +SetHash * Cartesian[Hash] * ContravariantCartesian[Hash] * ContravariantCartesian[Hash]; Hash.fromHashing * style issues * remove unused import * some test cases * some test cases * +test for Contravariant[Hash] * +test for Contravariant[Hash] * Fix duplication error and style error * fix merge error * remove instance for Cartesian[Hash]: it does not satisfy associativity * +identityHash, +`hash` postfix method * all tests passed * increase coverage * accidentally removed plugin, restore it * all tests passed * increase coverage * increase coverage, ## => hashCode, kernelBoiler * suppress mimaReportBinaryIssues * Remove cats.functor * Remove cats.functor

yilinwei force-pushed the fix-instances branch from dc57fab to 9a0e97b Compare May 11, 2017 10:48

move instances into separate trait

a644aaf

yilinwei force-pushed the fix-instances branch from 9a0e97b to a644aaf Compare May 11, 2017 10:51

yilinwei mentioned this pull request May 11, 2017

Kernel instances are in typeclasses which are subclasses #1657

Closed

kailuowang added the Source Breaking label May 11, 2017

adding separate objects for piecemeal imports

4775bd8

ceedubs mentioned this pull request May 13, 2017

Order to Ordering conversion #1665

Closed

yilinwei mentioned this pull request May 19, 2017

Consistency tests for override instances #1684

Open

kailuowang added this to the 1.0.0-MF milestone May 25, 2017

kailuowang mentioned this pull request May 25, 2017

Prepare for the 1.0.0-MF release #1702

Merged

26 tasks

Adding implicit resolution tests for piecemeal imports for hierarchy

4acff56

yilinwei force-pushed the fix-instances branch from db5b07f to 4acff56 Compare May 26, 2017 19:19

peterneyens reviewed May 28, 2017

View reviewed changes

kailuowang reviewed May 28, 2017

View reviewed changes

Removing explicit implicitly and using apply method

fea3b25

peterneyens approved these changes May 28, 2017

View reviewed changes

peterneyens merged commit b722f2b into typelevel:master May 28, 2017

LukaJCB pushed a commit to LukaJCB/cats that referenced this pull request Sep 8, 2017

Move contravariant instances to separate trait, in accordance with (t…

f645496

…ypelevel#1659)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move instances into separate trait #1659

move instances into separate trait #1659

yilinwei commented May 11, 2017 •

edited

Loading

codecov-io commented May 11, 2017 •

edited

Loading

ceedubs commented May 11, 2017

edmundnoble commented May 11, 2017

yilinwei commented May 12, 2017

ceedubs commented May 12, 2017

yilinwei commented May 13, 2017

yilinwei commented May 16, 2017 •

edited

Loading

peterneyens commented May 16, 2017

yilinwei commented May 16, 2017

kailuowang commented May 25, 2017

yilinwei commented May 25, 2017

djspiewak commented May 26, 2017

kailuowang commented May 26, 2017

peterneyens May 28, 2017

yilinwei May 28, 2017

kailuowang May 28, 2017

kailuowang May 28, 2017

kailuowang May 28, 2017

peterneyens commented May 28, 2017

move instances into separate trait #1659

move instances into separate trait #1659

Conversation

yilinwei commented May 11, 2017 • edited Loading

codecov-io commented May 11, 2017 • edited Loading

Codecov Report

ceedubs commented May 11, 2017

edmundnoble commented May 11, 2017

yilinwei commented May 12, 2017

ceedubs commented May 12, 2017

yilinwei commented May 13, 2017

yilinwei commented May 16, 2017 • edited Loading

peterneyens commented May 16, 2017

yilinwei commented May 16, 2017

kailuowang commented May 25, 2017

yilinwei commented May 25, 2017

djspiewak commented May 26, 2017

kailuowang commented May 26, 2017

peterneyens May 28, 2017

Choose a reason for hiding this comment

yilinwei May 28, 2017

Choose a reason for hiding this comment

kailuowang May 28, 2017

Choose a reason for hiding this comment

kailuowang May 28, 2017

Choose a reason for hiding this comment

kailuowang May 28, 2017

Choose a reason for hiding this comment

peterneyens commented May 28, 2017

yilinwei commented May 11, 2017 •

edited

Loading

codecov-io commented May 11, 2017 •

edited

Loading

yilinwei commented May 16, 2017 •

edited

Loading