-
-
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
Move catsTraverseForSeq
to lower-priority implicit scope
#4373
Conversation
Thanks!
How did you confirm this, were we getting the same warning in Cats itself?
Unfortunately the Cats build is currently full of warnings, so it's quite possible it's unrelated. |
@@ -98,9 +98,13 @@ trait UnorderedFoldable[F[_]] extends Serializable { | |||
unorderedFoldMap(fa)(a => if (p(a)) 1L else 0L) | |||
} | |||
|
|||
trait LowPriorityImplicits { |
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.
trait LowPriorityImplicits { | |
private trait UnorderedFoldableLowPriority { |
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 saw (late) that it couldn't be used later if it was private: private trait UnorderedFoldableLowPriority escapes its defining scope as part of type cats.UnorderedFoldableLowPriority
I changed it to protected, I think it makes sense, what do you think?
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
I replicated the error using the code from the issue, ran it in the repl and got the same warning that @bpholt mentions. Then made the change and ran it again without warnings. I didn't check if the same warning was anywhere in Cats itself |
catsTraverseForSeq
to lower-priority implicit scope
…ompile as private)
private trait UnorderedFoldableLowPriority { | ||
protected trait UnorderedFoldableLowPriority { |
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.
Hm, was there a compile error if this was private
? If so, we should make it private[cats]
, not protected
.
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.
Yep, it failed with private trait UnorderedFoldableLowPriority escapes its defining scope as part of type cats.UnorderedFoldableLowPriority
, I changed it to private[cats] and compiles. I'm running the prePR and I'll push it
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.
Thanks for the fix, much appreciated!
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.
LGTM
Hi, first (try)pr here. Trying to solve the bug at issue #4370.
I followed the idea to de-prioritize the Seq instance. Though it solves the issue mentioned, it prompts some other warnings at compilation time (I'm not sure it's specific to this change).
As told in the issue, I'll be reading a bit if there is another way around it, unless the solution is considered good enough.
Any ideas or directions are welcome.