cats icon indicating copy to clipboard operation
cats copied to clipboard

Implicit `catsKernelOrderingForOrder` is not very useful

Open armanbilge opened this issue 3 years ago • 7 comments

https://github.com/typelevel/cats/blob/21033a6428f0815d046979975f150455691b0b22/kernel/src/main/scala/cats/kernel/Order.scala#L136-L145

The problem is, when the compiler searches for an implicit Ordering[A], it has no reason to check the Order companion object. So it would have to be manually brought into scope e.g. import cats.kernel.Order._ or something.

armanbilge avatar Oct 31 '22 18:10 armanbilge

yeah, but if you want it you would import it right?

I think it would be dangerous to put it in cats.implicits._ although maybe it would be okay. I worry it could create ambiguous implicits and then look like no Ordering could be found.

johnynek avatar Oct 31 '22 20:10 johnynek

The cats.implicits._ import is deprecated now anyway, so I didn't really consider that an option.

Also cats.kernel.Order._ seems like a bad idea, because the wildcard will import lots of random stuff.

So actually the only realistic way to use this is to import cats.kernel.Order.catsKernelOrderingForOrder. All so you can avoid doing this somewhere in your code:

implicit val ordering = Order[A].toOrdering

Maybe that's fine and it's a matter of taste. But I took the latter option instead of the bulky import.

Edit: linking to

  • https://github.com/typelevel/cats/issues/2455

armanbilge avatar Oct 31 '22 21:10 armanbilge

when was cats.implicits._ deprecated? Oh it's that cats.syntax._ or something replacement right?

johnynek avatar Oct 31 '22 21:10 johnynek

Yup, that's right, it's cats.syntax.all._ now.

https://github.com/typelevel/cats/releases/tag/v2.2.0

In most cases all that's necessary to switch to using the new implicit scope instances is to replace cats.implicits._ imports with cats.syntax.all._ and delete any cats.instances imports

armanbilge avatar Oct 31 '22 21:10 armanbilge

import cats.kernel.Order.Implicits._

maybe or something like that?

Not too cool, but still better than just

import cats.kernel.Order._

satorg avatar Oct 31 '22 22:10 satorg

I think it would be dangerous to put it in cats.implicits._ although maybe it would be okay.

Chatting on Discord today Fabio points out that this instance is already in cats.implicits._. In fact, it's one of the few cases where the implicits import can't be replaced by the syntax import.

armanbilge avatar Aug 21 '23 16:08 armanbilge

import cats.syntax.all._ can still be used in combination with import cats.kernel.instances.order._, but it is not the best experience in terms of API discoverability of course.

UPD: changed "it is the best experience" to "NOT the best experience". Keep mistyping, sorry.

satorg avatar Aug 21 '23 17:08 satorg