Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose newtype objects to Mima #4219

Merged
merged 3 commits into from
Jun 11, 2022

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Jun 2, 2022

The motivation for this PR is discussed here: #4187 (comment)

@satorg satorg added this to the 2.8.0 milestone Jun 2, 2022
@satorg satorg requested a review from armanbilge June 2, 2022 18:38
@satorg satorg self-assigned this Jun 2, 2022
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from bf2194c to c6cc133 Compare June 9, 2022 04:02
@armanbilge armanbilge modified the milestone: 2.8.0 Jun 9, 2022
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from c6cc133 to 5cd610b Compare June 10, 2022 23:05
@satorg satorg requested a review from armanbilge June 10, 2022 23:07
@satorg
Copy link
Contributor Author

satorg commented Jun 10, 2022

Hmm... Looks like making the NonEmptySetImpt object public makes Mima absolutely unhappy:

[error] Cats core: Failed binary compatibility check against org.typelevel:cats-core_2.13:2.3.0 (e:info.apiURL=https://oss.sonatype.org/service/local/repositories/snapshots/archive/org/typelevel/cats-docs_2.13/2.8-5cd610b-SNAPSHOT/cats-docs_2.13-2.8-5cd610b-SNAPSHOT-javadoc.jar/!/index.html, e:info.versionScheme=early-semver)! Found 1 potential problems
[error]  * static method catsDataEqForNonEmptySet(cats.kernel.Order)cats.kernel.Eq in class cats.data.NonEmptySetImpl does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.data.NonEmptySetImpl.catsDataEqForNonEmptySet")
[error] stack trace is suppressed; run last coreJVM / mimaReportBinaryIssues for the full output
[error] Total time: 25 s, completed Jun 10, 2022 4:35:11 PM
[error] (coreJVM / mimaReportBinaryIssues) Failed binary compatibility check against org.typelevel:cats-core_2.13:2.3.0 (e:info.apiURL=https://oss.sonatype.org/service/local/repositories/snapshots/arc

Whereas the previouls solution (with abstract class) didn't concern Mima at all...

If I said that now I understand just nothing – it wouldn't be too far from the truth...

@armanbilge
Copy link
Member

Right, that's because as I said, we already broke binary compatibility. Now that you've made it public, MiMa is actually able to check bincompat :) this is why I think the is the right strategy forward.

See #4219 (comment) for a patch to fix it.

@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

Ah ok, now I got it (finally). Thanks.

@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 5cd610b to de5357b Compare June 11, 2022 01:32
@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

@armanbilge I've updated the PR, take one more look please.

@satorg satorg requested a review from armanbilge June 11, 2022 01:35
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.

👍 excellent scaladoc comment, thank you! Just a small typo :)

@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from de5357b to 5c2a0c0 Compare June 11, 2022 01:51
armanbilge
armanbilge previously approved these changes Jun 11, 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.

Thank you for identifying and investigating this issue so thoroughly!

@satorg satorg requested a review from armanbilge June 11, 2022 01:52
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 5c2a0c0 to 96966e4 Compare June 11, 2022 02:52
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 96966e4 to d6fb9df Compare June 11, 2022 03:42
@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

I think I can merge it now – it shouldn't make things worse somehow.

@satorg satorg merged commit 113da92 into typelevel:main Jun 11, 2022
@satorg satorg deleted the expose-newtype-objects-to-mima branch June 11, 2022 19:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants