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 a migration rule to help migration to 0.9.28 #1395

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -277,3 +277,42 @@ lazy val docs = project
.dependsOn(testkit, core, cli)
.enablePlugins(DocusaurusPlugin)
.disablePlugins(ScalafixPlugin)

lazy val `migration-rules` = project
.in(file("migration/rules"))
.settings(
libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % "0.9.27"
)

lazy val `migration-input` = project
.in(file("migration/input"))
.settings(
(publish / skip) := true,
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % "0.9.27" cross CrossVersion.full
)

lazy val `migration-output` = project
.in(file("migration/output"))
.settings(
(publish / skip) := true,
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % "0.9.27" cross CrossVersion.full
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure the output compiles with what will become 0.9.28?

Suggested change
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % "0.9.27" cross CrossVersion.full
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % "0.9.27+52-6c9eeec9-SNAPSHOT" cross CrossVersion.full
  • sonatype snapshot resolver

)

lazy val `migration-tests` = project
.in(file("migration/tests"))
.settings(
(publish / skip) := true,
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % "0.9.27" % Test cross CrossVersion.full,
(Compile / compile) :=
(Compile / compile)
.dependsOn((`migration-input` / Compile / compile))
.value,
scalafixTestkitOutputSourceDirectories :=
(`migration-output` / Compile / unmanagedSourceDirectories).value,
scalafixTestkitInputSourceDirectories :=
(`migration-input` / Compile / unmanagedSourceDirectories).value,
scalafixTestkitInputClasspath :=
(`migration-input` / Compile / fullClasspath).value
)
.dependsOn(`migration-rules`)
.enablePlugins(ScalafixTestkitPlugin)
10 changes: 10 additions & 0 deletions migration/input/src/main/scala/fix/OldSemanticRule.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
rule = v0_9_28
*/
package fix

import scalafix.testkit.SemanticRuleSuite

class OldSemanticRule extends SemanticRuleSuite() {
runAllTests()
}
8 changes: 8 additions & 0 deletions migration/output/src/main/scala/fix/OldSemanticRule.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package fix

import org.scalatest.FunSuiteLike
import scalafix.testkit.AbstractSemanticRuleSuite

class OldSemanticRule extends AbstractSemanticRuleSuite with FunSuiteLike {
runAllTests()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix.v0_9_28
22 changes: 22 additions & 0 deletions migration/rules/src/main/scala/fix/v0_9_28.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package fix

import scala.meta._

import scalafix.v1._

class v0_9_28 extends SemanticRule("v0_9_28") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this migration is for the rule authors (testkit) and not end users (cli/interface), might be worth mentioning it in the name? also a link to the PRs that introduced/enforced the pattern applied here would be useful

Suggested change
class v0_9_28 extends SemanticRule("v0_9_28") {
// https://github.com/scalacenter/scalafix/pull/1176
// https://github.com/scalacenter/scalafix/pull/1379
class Testkit_0_9_28 extends SemanticRule("testkit_0_9_28") {

Copy link
Collaborator

Choose a reason for hiding this comment

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

might be worth adding a link to #1176 for the record

override def fix(implicit doc: SemanticDocument): Patch =
doc.tree.collect {
case t @ Importee.Name(Name("SemanticRuleSuite")) =>
Patch.removeImportee(t) +
Patch.addGlobalImport(
importer"scalafix.testkit.AbstractSemanticRuleSuite"
)
case t @ Init(_, _, _) =>
if (t.toString() == "SemanticRuleSuite()")
Copy link
Collaborator

Choose a reason for hiding this comment

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

use semanticdb information through SymbolMatcher rather than relying on representation (the empty parameters list being optional, this will yield false negatives).

Patch.addGlobalImport(importer"org.scalatest.FunSuiteLike") +
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just had a double-check on the mess that ScalaTest introduced in a minor release:

  • org.scalatest.FunSuiteLike is available for ScalaTest < 3.2.0
  • org.scalatest.funsuite.AnyFunSuite is available for ScalaTest >= 3.1.0

We currently build against 3.0.8, so the first is the right choice, and users of the testkit which have evicted 3.0.8 with 3.2.x would have runtime failures on SemanticRuleSuite anyway, so this migration does not apply to them.

We will easily be able to move to org.scalatest.funsuite.AnyFunSuite via another migration (at least where Steward runs) for a further version where we bump to 3.2.0 (or something else if we switch to munit).

Patch.replaceTree(t, "AbstractSemanticRuleSuite") +
Patch.addRight(t, " with FunSuiteLike")
Comment on lines +18 to +19
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not

Suggested change
Patch.replaceTree(t, "AbstractSemanticRuleSuite") +
Patch.addRight(t, " with FunSuiteLike")
Patch.replaceTree(t, "AbstractSemanticRuleSuite with FunSuiteLike")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I guess it does not matter since chances that AbstractSemanticRuleSuite was mixing-in other traits are very low, but the rule would erase the existing ones - to be correct, you could have prettyprinted a patched version of t instead.

else Patch.empty
}.asPatch
}
8 changes: 8 additions & 0 deletions migration/tests/src/test/scala/fix/RuleSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package fix

import org.scalatest.FunSuiteLike
import scalafix.testkit.AbstractSemanticRuleSuite

class RuleSuite extends AbstractSemanticRuleSuite with FunSuiteLike {
runAllTests()
}