-
Notifications
You must be signed in to change notification settings - Fork 186
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
Move internal code to internal package, fixes #192. #236
Conversation
This refactoring is help setup MiMa binary compatibility checks. There are no binary compatibility promises for code under the scalafix.internal package, while code outside the internal package will remain binary compatible across each 0.x series.
This commit sets up mima to check for binary compatibility issues against the latest official scalafix release. Unfortunately, mima does not support regexes to filter out issues in the internal package. There are currently 84 breaking changes from scalafix 0.4.2, almost all of them are classes that have now been moved to the internal package. I propose we enable mima in CI after the 0.5.0 release.
project/Mima.scala
Outdated
object Mima { | ||
val ignoredABIProblems: Seq[ProblemFilter] = { | ||
Seq( | ||
exclude[MissingClassProblem]("scalafix.package$XtensionRewriteCtx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with MiMa, can you please explain what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a way to document binary breaking changes. sbt mimaReportBinaryIssues
compares the classfiles of the current HEAD with the classfiles of the latest published release to Maven and reports issues such as "missing class/method". It's possible to ignore those issues by acknowledging them like this, to indicate you the breaking change is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the v0.5.0 release, we can run mimaReportBinaryIssues
in CI so that PRs will need to document the breaking changes in the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. And what about this specific line?
Why are we ignoring that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really shouldn't be there, I kept it as an example of how to filter out errors. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Update scalafmt-core to 3.1.1 * Reformat with scalafmt 3.1.1
This PR is a followup of #193.