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 syntax <&> for parallel #2671

Merged
merged 4 commits into from
Dec 19, 2018
Merged

Conversation

jcouyang
Copy link
Contributor

in #2038 introduced &> and <& syntax for parallel which are very helpful

why not have a <&> too, so I can

mab <&> ma

instead of

Parallel.parAp(mab)(ma)

@@ -3,6 +3,9 @@ package cats.syntax
import cats.{FlatMap, Foldable, Monad, Parallel, Traverse}

trait ParallelSyntax extends TupleParallelSyntax {
implicit final def catsSyntaxParallelApply[F[_], A, B](fa: F[A => B]): ParallelApplyOps[F, A, B] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, you can't add a method to a trait without breaking binary compatibility in Scala 2.11.
You need to add a new trait to add this method and have relevant objects extending it. And then you can have AllSyntaxBinCompat4 (a new trait introduced post 1.5.0 release) extend this.

@codecov-io
Copy link

codecov-io commented Dec 18, 2018

Codecov Report

Merging #2671 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2671      +/-   ##
==========================================
+ Coverage   95.12%   95.12%   +<.01%     
==========================================
  Files         364      365       +1     
  Lines        6707     6753      +46     
  Branches      285      285              
==========================================
+ Hits         6380     6424      +44     
- Misses        327      329       +2
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/parallel.scala 70.58% <0%> (-9.42%) ⬇️
...estkit/src/main/scala/cats/tests/ListWrapper.scala 100% <0%> (ø)

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 b329cf1...430dda0. Read the comment docs.

@kubukoz
Copy link
Member

kubukoz commented Dec 18, 2018

I think you can add your syntax to the trait introduced here (in a similar way): #2600 (it was after 1.5.0)

@jcouyang
Copy link
Contributor Author

thanks @kailuowang @kubukoz I added it to AllSyntaxBinCompat4 also the syntax object parallel
but I think the coverage drop because SyntaxSuite.scala is only compile time check, coverage report was not able picking that up
should I add a run time test case in ParallelSuite.scala?

@kubukoz
Copy link
Member

kubukoz commented Dec 19, 2018

I don't think anyone looks at the coverage reports, but if you want you can add a doctest to ensure this works as expected (I'm not sure syntax is being tested in ParallelSuite)

@jcouyang
Copy link
Contributor Author

Good, I personally think the syntaxsuite is good enough for covering the syntax

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.

Looks good to me

@LukaJCB LukaJCB merged commit d250587 into typelevel:master Dec 19, 2018
@jcouyang jcouyang deleted the parallelsyntax branch December 20, 2018 01:19
@kailuowang kailuowang added this to the 1.6 milestone Jan 8, 2019
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