-
-
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
Added parFlatMapN
#4243
Merged
Merged
Added parFlatMapN
#4243
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
a0e0efb
Added flatParMapN
TonioGela fe96ffa
Added flatParMapN tests for 1-arity
TonioGela f62d98b
Refactored .map.flatten in to .flatmap
TonioGela 997e803
Modified function name as per discussion
TonioGela 06ac73b
Added ParallelSuite tests for parFlatMapN
TonioGela 5e41658
Changed implementation using arity functions
TonioGela adbb470
Rewrote test for clarity
TonioGela c8b6cd0
Autogenerating the test function itself
TonioGela 325b4b4
Generalizing all the parFlatMap tests
TonioGela File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not directly about this PR, but I'm confused by the code here (also for the existing method above it). Why is it calling
p.flatMap.map
instead ofp.apply.map
? Since theApply
is the parallel instance.Am I confused or is this a bug? Maybe we need some tests using e.g.
EitherNec[String, A]
?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 agree, good catch... So now I wonder too.
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.
Ahh, no this is completely fine. It's because it's calling vanilla
map
on a single argument (a tuple), which is then unpacked by thecase
in the lambda. I was mistakenly reading it as calling amapN
-like method, in which case this would be wrong.The
map
must be consistent between theflatMap
and theapply
by law, and using theapply
instance would require a conversion to the parallel datatype and back. So it's an optimization to use the one from theflatMap
instead.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.
Actually ... this is just
flatMap
... right? 😂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.
It is! I run the tests locally just to be sure that I was not missing something syntax related :)