Skip to content

weaken constraint on ParallelApOps and ParallelApplyOps methods - #4078

Merged
armanbilge merged 11 commits into
typelevel:mainfrom
jbwheatley:#4077-parallel-syntax
May 25, 2022
Merged

weaken constraint on ParallelApOps and ParallelApplyOps methods#4078
armanbilge merged 11 commits into
typelevel:mainfrom
jbwheatley:#4077-parallel-syntax

Conversation

@jbwheatley

Copy link
Copy Markdown
Contributor

Addresses #4077, following the technique in #3997 to ensure bincompat

Comment thread core/src/main/scala/cats/syntax/parallel.scala Outdated
@satorg

satorg commented Dec 9, 2021

Copy link
Copy Markdown
Contributor

It is all about syntax changes only, right? I think it may be worth to fix SyntaxSuite according to these changes too.

Comment on lines +38 to +40

implicit final def catsSyntaxParallelAp1[M[_], A](ma: M[A]): ParallelApOps[M, A] =
new ParallelApOps(ma)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FlatMap also didn't get used so have removed it as a constraint for using the NonEmptyParallel syntax

Comment on lines 35 to 36
@deprecated("Kept for binary compatibility", "2.8.0")
implicit final def catsSyntaxParallelAp[M[_]: FlatMap, A](ma: M[A]): ParallelApOps[M, A] =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
@deprecated("Kept for binary compatibility", "2.8.0")
implicit final def catsSyntaxParallelAp[M[_]: FlatMap, A](ma: M[A]): ParallelApOps[M, A] =
@deprecated("Kept for binary compatibility", "2.8.0")
private[syntax] final def catsSyntaxParallelAp[M[_]: FlatMap, A](ma: M[A]): ParallelApOps[M, A] =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you reverted this because MiMa was complaining about missing static methods? I think it is is safe to filter those, because they only affect Java and I think it would be unusual for Java code to be using these syntax extensions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it may be okay to leave it public if private[syntax] breaks BC. The only thing – the implicit modifier should be pulled off the identifier, but as I can see, it is already done here.

Comment thread core/src/main/scala/cats/syntax/parallel.scala Outdated
Comment thread core/src/main/scala/cats/syntax/parallel.scala Outdated
Comment thread core/src/main/scala/cats/syntax/parallel.scala Outdated
Comment thread tests/src/test/scala/cats/tests/SyntaxSuite.scala
satorg
satorg previously approved these changes Dec 14, 2021

@satorg satorg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks really good to me. Thank you for investing your time into it.

@jbwheatley

Copy link
Copy Markdown
Contributor Author

any clue why the tests aren't compiling now? can't spot it and intellij is happy 🤔

@satorg

satorg commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Looks like the tests are failing for Scala 2.12.15, but IntelliJ compiles with Scala 2.13.7 as this is a default Scala version.

@satorg

satorg commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Try sbt ++2.12.15 testsJVM/test command

@jbwheatley

Copy link
Copy Markdown
Contributor Author

latest commit seems to be necessary to get the syntax to work on 2.12.15

@armanbilge armanbilge added this to the 2.8.0 milestone May 16, 2022

@armanbilge armanbilge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for all your effort to fix this bincompatibly, much appreciated! :)

@armanbilge
armanbilge requested a review from satorg May 23, 2022 20:29

@satorg satorg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm, thanks!

@armanbilge
armanbilge merged commit 4e211c5 into typelevel:main May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Most methods on ParallelApOps and ParallelApplyOps should only require a NonEmptyParallel instance

3 participants