-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cats.implicits._
-> cats.syntax.all._
#4394
cats.implicits._
-> cats.syntax.all._
#4394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you 👍
import cats.instances.all._ | ||
import cats.syntax.all._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. That looks quite odd. As I understand, these two imports should not be required to happen simultaneously. So if they are required then perhaps some implicits are missing from the cats.syntax.all._
path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if they are required then perhaps some implicits are missing from the
No, odd but correct. How can you resolve this implicit with only the syntax import?
cats.Monad[cats.data.OptionT[List, *]], |
The syntax imports only work if you use the syntax :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Not sure, what do you mean. This particular example does not require any additional imports to compile.
Because Monad
for OptionT
is picked up from the OptionT
's companion, apparently.
Whereas Monad[List]
is picked up from the Invariant
's companion automatically:
cats/core/src/main/scala/cats/Invariant.scala
Lines 138 to 139 in ccd576f
implicit def catsInstancesForList: Monad[List] with Alternative[List] with CoflatMap[List] = | |
cats.instances.list.catsStdInstancesForList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, I don't think it is an obstacle to get this PR merged :) Thank you for cleaning the imports up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hm maybe I said a stupid :) and I was so sure I had it all figured out! let me take one more look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh 🤦 it's because the whole point of this file, it to compile it against Cats v2.0.0, which pre-dates the re-organization that puts all implicits automatically in scope.
Lines 265 to 271 in ccd576f
lazy val binCompatTest = project | |
.enablePlugins(NoPublishPlugin) | |
.settings( | |
useCoursier := false, // workaround so we can use an old version in compile | |
libraryDependencies += { | |
val oldV = if (tlIsScala3.value) "2.6.1" else "2.0.0" | |
"org.typelevel" %%% "cats-core" % oldV % Provided |
Ok, sorry, and thanks 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to revert to cats.implicits
for this one. The point is to make sure we don't break compatibility with old code, so it seems better to write that import the old way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it makes sense, thanks!
It's about damn time!