-
-
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
Add Parallel traverseFilter functions #3467
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3467 +/- ##
==========================================
- Coverage 91.59% 91.37% -0.23%
==========================================
Files 382 383 +1
Lines 8319 8776 +457
Branches 220 216 -4
==========================================
+ Hits 7620 8019 +399
- Misses 699 757 +58 |
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.
Looks good to me!
@@ -100,6 +113,19 @@ final class ParallelTraversableOps[T[_], A](private val ta: T[A]) extends AnyVal | |||
|
|||
} | |||
|
|||
final class ParallelTraverseFilterOps[T[_], A](private val ta: T[A]) extends AnyVal { |
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 think it'd be better to consolidate these, so that all enrichment methods for T[A]
are on a single enrichment class, mostly in order to reduce the number of implicits in cats.syntax.all
, but also just to simplify this code. In this case that would mean putting these methods on something named ParallelTraversableOps
or ParallelFoldMapAOps
, though, which also seems kind of bad.
I think for now this version is probably best, but we should definitely revisit this in Cats 3.
this PR brought to you by https://www.youtube.com/watch?v=SMMgwfhPCzM&feature=youtu.be 🙂 |
You are fast @LukaJCB Thanks for the addition :) |
@@ -94,4 +94,4 @@ trait AllSyntaxBinCompat4 | |||
|
|||
trait AllSyntaxBinCompat5 extends ParallelBitraverseSyntax | |||
|
|||
trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax | |||
trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax with ParallelTraverseFilterSyntax |
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.
Forgive me for being out for a while, but just curious why we still extend a binCompat trait instead of the AllSyntax
itself?
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.
Ah, good catch. I agree that this should go directly on AllSyntax
, as in #3392.
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.
ah of course, thanks!
@@ -94,4 +94,4 @@ trait AllSyntaxBinCompat4 | |||
|
|||
trait AllSyntaxBinCompat5 extends ParallelBitraverseSyntax | |||
|
|||
trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax | |||
trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax with ParallelTraverseFilterSyntax |
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.
Ah, good catch. I agree that this should go directly on AllSyntax
, as in #3392.
👍 on green, but I think we still need one more review. |
Thanks @barambani 😊 merging this |
Should fix #3465