From 0ab2a67f9b74b730dfa3ec074f72079377eb94d8 Mon Sep 17 00:00:00 2001 From: Alex Klibisz Date: Mon, 9 Jan 2023 16:31:52 -0800 Subject: [PATCH 1/3] Issue #2906: Misleading warning when two deps reference the same val version --- .../scalasteward/core/edit/RewriteTest.scala | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala index 045971eb4a..3612f1cac2 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala @@ -7,6 +7,7 @@ import org.scalasteward.core.TestSyntax._ import org.scalasteward.core.data.{RepoData, Update} import org.scalasteward.core.mock.MockContext.context._ import org.scalasteward.core.mock.MockState +import org.scalasteward.core.mock.MockState.TraceEntry import org.scalasteward.core.repoconfig.RepoConfig import org.scalasteward.core.scalafmt.scalafmtConfName import org.scalasteward.core.util.Nel @@ -911,6 +912,46 @@ class RewriteTest extends FunSuite { runApplyUpdate(update, original, expected) } + // Sketchy placeholder bI'm not sure how else to assert on the presence/absence of a log. + private val terribleMutableLogBufferDoNotMergeThis = + scala.collection.mutable.ListBuffer.empty[String] + + test("issue ...") { + val update1 = ("com.foo".g % "library-1".a % "1.2.3" %> "1.3.0").single + val update2 = ("com.foo".g % "library-2".a % "1.2.3" %> "1.3.0").single + val original = Map( + "build.sbt" -> + """ + |val FooLibraryVersion = "1.2.3" + |libraryDependencies ++= Seq( + | "com.foo" %% "library-1" % FooLibraryVersion, + | "com.foo" %% "library-2" % FooLibraryVersion + |) + |""".stripMargin + ) + val expected = Map( + "build.sbt" -> + """ + |val FooLibraryVersion = "1.3.0" + |libraryDependencies ++= Seq( + | "com.foo" %% "library-1" % FooLibraryVersion, + | "com.foo" %% "library-2" % FooLibraryVersion + |) + |""".stripMargin + ) + + runApplyUpdate(update1, original, expected) + require( + !terribleMutableLogBufferDoNotMergeThis.exists(_.startsWith("Unable to bump version")) + ) + + // The second update prints a misleading warning. + runApplyUpdate(update2, expected, expected) + require( + terribleMutableLogBufferDoNotMergeThis.last == "Unable to bump version for update com.foo:library-2 : 1.2.3 -> 1.3.0" + ) + } + private def runApplyUpdate( update: Update.Single, files: Map[String, String], @@ -924,6 +965,12 @@ class RewriteTest extends FunSuite { .addFiles(filesInRepoDir.toSeq: _*) .flatMap(editAlg.applyUpdate(data, update).runS) .unsafeRunSync() + terribleMutableLogBufferDoNotMergeThis.clear() + state.trace.foreach { + case l: TraceEntry.Log => + terribleMutableLogBufferDoNotMergeThis += l.log._2 + case _ => () + } val obtained = state.files .map { case (file, content) => file.toString.replace(repoDir.toString + "/", "") -> content } assertEquals(obtained, expected) From 39b618f79d1c850baba647018614f67b38674a8f Mon Sep 17 00:00:00 2001 From: Alex Klibisz Date: Sun, 12 Feb 2023 09:31:13 -0800 Subject: [PATCH 2/3] Println debugging --- .../org/scalasteward/core/edit/EditAlg.scala | 79 +++++++++++-------- .../core/nurture/NurtureAlg.scala | 1 + 2 files changed, 49 insertions(+), 31 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala index 4ff9e17b4b..f4e933bdfb 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala @@ -50,22 +50,27 @@ final class EditAlg[F[_]](implicit update: Update.Single, preCommit: F[Unit] = F.unit ): F[List[EditAttempt]] = - findUpdateReplacements(data.repo, data.config, update).flatMap { - case Nil => logger.warn(s"Unable to bump version for update ${update.show}").as(Nil) - case updateReplacements => - for { - _ <- preCommit - (preMigrations, postMigrations) = scalafixMigrationsFinder.findMigrations(update) - preScalafixEdits <- runScalafixMigrations(data.repo, data.config, preMigrations) - // PreUpdate migrations could invalidate previously found replacements, - // so we find them again if the migrations produced any changes. - freshReplacements <- - if (preScalafixEdits.flatMap(_.commits).isEmpty) F.pure(updateReplacements) - else findUpdateReplacements(data.repo, data.config, update) - updateEdit <- applyUpdateReplacements(data, update, freshReplacements) - postScalafixEdits <- runScalafixMigrations(data.repo, data.config, postMigrations) - hooksEdits <- hookExecutor.execPostUpdateHooks(data, update) - } yield preScalafixEdits ++ updateEdit ++ postScalafixEdits ++ hooksEdits + { + findUpdateReplacements(data.repo, data.config, update).flatMap { + case Nil => + logger.warn(s"Unable to bump version for update ${update.show}").as(Nil) + case updateReplacements => + for { + _ <- preCommit + (preMigrations, postMigrations) = scalafixMigrationsFinder.findMigrations(update) + preScalafixEdits <- runScalafixMigrations(data.repo, data.config, preMigrations) + // PreUpdate migrations could invalidate previously found replacements, + // so we find them again if the migrations produced any changes. + freshReplacements <- + if (preScalafixEdits.flatMap(_.commits).isEmpty) F.pure(updateReplacements) + else findUpdateReplacements(data.repo, data.config, update) + updateEdit <- applyUpdateReplacements(data, update, freshReplacements) + postScalafixEdits <- runScalafixMigrations(data.repo, data.config, postMigrations) + hooksEdits <- hookExecutor.execPostUpdateHooks(data, update) + } yield { + preScalafixEdits ++ updateEdit ++ postScalafixEdits ++ hooksEdits + } + } } private def findUpdateReplacements( @@ -73,10 +78,16 @@ final class EditAlg[F[_]](implicit config: RepoConfig, update: Update.Single ): F[List[Substring.Replacement]] = - for { - versionPositions <- scannerAlg.findVersionPositions(repo, config, update.currentVersion) - modulePositions <- scannerAlg.findModulePositions(repo, config, update.dependencies) - } yield Selector.select(update, versionPositions, modulePositions) + { + println("findUpdateReplacements") + println(s" update = $update") + for { + versionPositions <- scannerAlg.findVersionPositions(repo, config, update.currentVersion) + _ = println(s" versionPositions = ${versionPositions}") + modulePositions <- scannerAlg.findModulePositions(repo, config, update.dependencies) + _ = println(s" modulePositions = ${modulePositions}") + } yield Selector.select(update, versionPositions, modulePositions) + } private def runScalafixMigrations( repo: Repo, @@ -103,17 +114,23 @@ final class EditAlg[F[_]](implicit update: Update.Single, updateReplacements: List[Substring.Replacement] ): F[Option[EditAttempt]] = - for { - repoDir <- workspaceAlg.repoDir(data.repo) - replacementsByPath = updateReplacements.groupBy(_.position.path).toList - _ <- replacementsByPath.traverse { case (path, replacements) => - fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements)) - } - _ <- reformatChangedFiles(data) - msgTemplate = data.config.commits.messageOrDefault - commitMsg = CommitMsg.replaceVariables(msgTemplate)(update, data.repo.branch) - maybeCommit <- gitAlg.commitAllIfDirty(data.repo, commitMsg) - } yield maybeCommit.map(UpdateEdit(update, _)) + { + println("applyUpdateReplacements") + println(s" data = ${data}") + println(s" update = ${update}") + println(s" updateReplacements = ${updateReplacements}") + for { + repoDir <- workspaceAlg.repoDir(data.repo) + replacementsByPath = updateReplacements.groupBy(_.position.path).toList + _ <- replacementsByPath.traverse { case (path, replacements) => + fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements)) + } + _ <- reformatChangedFiles(data) + msgTemplate = data.config.commits.messageOrDefault + commitMsg = CommitMsg.replaceVariables(msgTemplate)(update, data.repo.branch) + maybeCommit <- gitAlg.commitAllIfDirty(data.repo, commitMsg) + } yield maybeCommit.map(UpdateEdit(update, _)) + } private def reformatChangedFiles(data: RepoData): F[Unit] = { val reformat = diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala index 1145cba42b..302106ed17 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala @@ -93,6 +93,7 @@ final class NurtureAlg[F[_]](config: VCSCfg)(implicit _ <- logger.info(s"Process update ${data.update.show}") head = vcs.listingBranch(config.tpe, data.fork, data.updateBranch) pullRequests <- vcsApiAlg.listPullRequests(data.repo, head, data.baseBranch) + .map(_.take(0)) // Debug result <- pullRequests.headOption match { case Some(pr) if pr.state.isClosed && data.update.isInstanceOf[Update.Single] => logger.info(s"PR ${pr.html_url} is closed") >> From 5bb0ecc95af9b82be3c20dc60c22bca6393a4e48 Mon Sep 17 00:00:00 2001 From: Alex Klibisz Date: Wed, 22 Feb 2023 14:23:45 -0800 Subject: [PATCH 3/3] WIP --- .../core/nurture/NurtureAlg.scala | 4 + .../scalasteward/core/edit/RewriteTest.scala | 84 +++++++++++++------ 2 files changed, 62 insertions(+), 26 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala index 302106ed17..a6522bd346 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala @@ -162,6 +162,8 @@ final class NurtureAlg[F[_]](config: VCSCfg)(implicit gitAlg.returnToCurrentBranch(data.repo) { val createBranch = logger.info(s"Create branch ${data.updateBranch.name}") >> gitAlg.createBranch(data.repo, data.updateBranch) + println("applyNewUpdate") + println(s" data.update = ${data.update}") data.update .on( update = editAlg.applyUpdate(data.repoData, _, createBranch), @@ -296,6 +298,8 @@ final class NurtureAlg[F[_]](config: VCSCfg)(implicit ) maybeRevertCommit <- gitAlg.revertChanges(data.repo, data.baseBranch) maybeMergeCommit <- gitAlg.mergeTheirs(data.repo, data.baseBranch) + _ = println("mergeAndApplyAgain") + _ = println(s" data.update = ${data.update}") edits <- data.update.on( update = editAlg.applyUpdate(data.repoData, _), grouped = _.updates.flatTraverse(editAlg.applyUpdate(data.repoData, _)) diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala index 3612f1cac2..3389d80b8a 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/edit/RewriteTest.scala @@ -1,10 +1,12 @@ package org.scalasteward.core.edit +import cats.data.NonEmptyList import cats.effect.unsafe.implicits.global import munit.FunSuite import org.scalasteward.core.TestInstances.dummyRepoCache import org.scalasteward.core.TestSyntax._ -import org.scalasteward.core.data.{RepoData, Update} +import org.scalasteward.core.data.Update.ForArtifactId +import org.scalasteward.core.data.{ArtifactId, CrossDependency, Dependency, GroupId, RepoData, Update, Version} import org.scalasteward.core.mock.MockContext.context._ import org.scalasteward.core.mock.MockState import org.scalasteward.core.mock.MockState.TraceEntry @@ -912,13 +914,7 @@ class RewriteTest extends FunSuite { runApplyUpdate(update, original, expected) } - // Sketchy placeholder bI'm not sure how else to assert on the presence/absence of a log. - private val terribleMutableLogBufferDoNotMergeThis = - scala.collection.mutable.ListBuffer.empty[String] - - test("issue ...") { - val update1 = ("com.foo".g % "library-1".a % "1.2.3" %> "1.3.0").single - val update2 = ("com.foo".g % "library-2".a % "1.2.3" %> "1.3.0").single + test("issue 2906") { val original = Map( "build.sbt" -> """ @@ -940,23 +936,64 @@ class RewriteTest extends FunSuite { |""".stripMargin ) - runApplyUpdate(update1, original, expected) - require( - !terribleMutableLogBufferDoNotMergeThis.exists(_.startsWith("Unable to bump version")) - ) - - // The second update prints a misleading warning. - runApplyUpdate(update2, expected, expected) - require( - terribleMutableLogBufferDoNotMergeThis.last == "Unable to bump version for update com.foo:library-2 : 1.2.3 -> 1.3.0" - ) - } + val update = ForArtifactId( + crossDependency = CrossDependency( + dependencies = + NonEmptyList(Dependency(GroupId("com.foo"), ArtifactId("library-1"), Version("1.2.3")), List.empty) + ), + newerVersions = NonEmptyList(Version("1.3.0"), List.empty)) + + val trace = runApplyUpdate(update, original, expected) + trace.foreach(println) + } + +// test("issue ...") { +// val update1: Update.ForArtifactId = ("com.foo".g % "library-1".a % "1.2.3" %> "1.3.0").single +// val update2: Update.ForArtifactId = ("com.foo".g % "library-2".a % "1.2.3" %> "1.3.0").single +// val update3 = Update.ForGroupId(NonEmptyList(update1, List(update2))) +// val original = Map( +// "build.sbt" -> +// """ +// |val FooLibraryVersion = "1.2.3" +// |libraryDependencies ++= Seq( +// | "com.foo" %% "library-1" % FooLibraryVersion, +// | "com.foo" %% "library-2" % FooLibraryVersion +// |) +// |""".stripMargin +// ) +// val expected = Map( +// "build.sbt" -> +// """ +// |val FooLibraryVersion = "1.3.0" +// |libraryDependencies ++= Seq( +// | "com.foo" %% "library-1" % FooLibraryVersion, +// | "com.foo" %% "library-2" % FooLibraryVersion +// |) +// |""".stripMargin +// ) +// +// println("update1") +// runApplyUpdate(update1, original, expected) +// require( +// !terribleMutableLogBufferDoNotMergeThis.exists(_.startsWith("Unable to bump version")) +// ) +// +// // The second update prints a misleading warning. +// println("update2") +// runApplyUpdate(update2, expected, expected) +// require( +// terribleMutableLogBufferDoNotMergeThis.last == "Unable to bump version for update com.foo:library-2 : 1.2.3 -> 1.3.0" +// ) +// +// println("update3") +// runApplyUpdate(update3, original, expected) +// } private def runApplyUpdate( update: Update.Single, files: Map[String, String], expected: Map[String, String] - ): Unit = { + ): Vector[TraceEntry] = { val repo = Repo("edit-alg", s"runApplyUpdate-${nextInt()}") val data = RepoData(repo, dummyRepoCache, RepoConfig.empty) val repoDir = workspaceAlg.repoDir(repo).unsafeRunSync() @@ -965,15 +1002,10 @@ class RewriteTest extends FunSuite { .addFiles(filesInRepoDir.toSeq: _*) .flatMap(editAlg.applyUpdate(data, update).runS) .unsafeRunSync() - terribleMutableLogBufferDoNotMergeThis.clear() - state.trace.foreach { - case l: TraceEntry.Log => - terribleMutableLogBufferDoNotMergeThis += l.log._2 - case _ => () - } val obtained = state.files .map { case (file, content) => file.toString.replace(repoDir.toString + "/", "") -> content } assertEquals(obtained, expected) + state.trace } private var counter = 0