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 parTuple #2183

Merged
merged 8 commits into from
Mar 12, 2018
Merged

Add parTuple #2183

merged 8 commits into from
Mar 12, 2018

Conversation

barambani
Copy link
Contributor

As from the discussion in here #2128 this and attempt to add parTuple syntax in a way that preserves backwards binary compatibility.

@iravid
Copy link
Contributor

iravid commented Mar 6, 2018

This is awesome :-) could you maybe keep it as parTupled to be consistent with (a, b).tupled?

@iravid iravid mentioned this pull request Mar 6, 2018
@barambani
Copy link
Contributor Author

absolutely right, I will change it. Thanks

@codecov-io
Copy link

codecov-io commented Mar 6, 2018

Codecov Report

Merging #2183 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2183   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         330      330           
  Lines        5568     5568           
  Branches      201      207    +6     
=======================================
  Hits         5276     5276           
  Misses        292      292
Impacted Files Coverage Δ
core/src/main/scala/cats/Parallel.scala 88.46% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9135035...94d069b. Read the comment docs.

| * @groupdesc ParTupleArity Higher-arity parTuple methods
| * @groupprio ParTupleArity 999
| */
|trait ParallelArityFunctions2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this one an abstract class then we can add more methods in the future without breaking bin compat.

Copy link
Contributor Author

@barambani barambani Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, changed. To avoid problems mixing in the two with Parallel I extended ParallelArityFunctions with ParallelArityFunctions2. Do you think this will prevent from adding methods in the future ? Thanks

@barambani
Copy link
Contributor Author

It looks like there are some memory issues in the CI. Do I have ways to trigger it again ? I think it should pass, I checked MiMa locally and should be fine.

kailuowang
kailuowang previously approved these changes Mar 6, 2018
@kailuowang kailuowang added this to the 1.1 milestone Mar 7, 2018
@LukaJCB
Copy link
Member

LukaJCB commented Mar 7, 2018

Thanks very much for this @barambani! Just a tiny last request, can we add some sort of test for this? I'm pretty confident this works as is, but should we ever change anything it'd be good to have that extra test :)

@barambani
Copy link
Contributor Author

Thank you all

@barambani
Copy link
Contributor Author

Sorry I missed the edit. I will add the tests 👍

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks much!

@barambani
Copy link
Contributor Author

Thank you gents for the help 👍

@kailuowang kailuowang merged commit 2ee184e into typelevel:master Mar 12, 2018
@barambani barambani deleted the add-parTupled branch March 12, 2018 22:42
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.

5 participants