Skip to content
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

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Jun 10, 2020

Should fix #3465

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2020

Codecov Report

Merging #3467 into master will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@            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     

Copy link
Contributor

@travisbrown travisbrown left a 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 {
Copy link
Contributor

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.

@travisbrown travisbrown mentioned this pull request Jun 11, 2020
@SethTisue
Copy link
Member

SethTisue commented Jun 11, 2020

this PR brought to you by https://www.youtube.com/watch?v=SMMgwfhPCzM&feature=youtu.be 🙂

@saraiva132
Copy link
Contributor

You are fast @LukaJCB Thanks for the addition :)

@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 12, 2020
@@ -94,4 +94,4 @@ trait AllSyntaxBinCompat4

trait AllSyntaxBinCompat5 extends ParallelBitraverseSyntax

trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax
trait AllSyntaxBinCompat6 extends ParallelUnorderedTraverseSyntax with ParallelTraverseFilterSyntax
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

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.

@travisbrown
Copy link
Contributor

👍 on green, but I think we still need one more review.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 16, 2020

Thanks @barambani 😊 merging this

@LukaJCB LukaJCB merged commit 2537bfd into master Jun 16, 2020
@LukaJCB LukaJCB deleted the parallel-traverse-filter branch June 16, 2020 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add parallel variant for TraverseFilter
7 participants