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

weaken constraint on ParallelApOps and ParallelApplyOps methods #4078

Merged
merged 11 commits into from
May 25, 2022

Conversation

jbwheatley
Copy link
Contributor

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

@satorg
Copy link
Contributor

satorg commented Dec 9, 2021

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

satorg
satorg previously approved these changes Dec 14, 2021
Copy link
Contributor

@satorg satorg left a comment

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
Contributor Author

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

@satorg
Copy link
Contributor

satorg commented Dec 14, 2021

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

satorg commented Dec 14, 2021

Try sbt ++2.12.15 testsJVM/test command

@jbwheatley
Copy link
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
Copy link
Member

@armanbilge armanbilge left a comment

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

@satorg satorg left a comment

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