Repository: scala-steward-org/scala-steward (scala)

Issue:
Run scalafmt on every update?
We over-ambitiously keep our build.sbt files formatted through scalafmt. Often times it happens that a library version string gets longer/shorter and PR builds fail on scalafmt since the alignment of sbt configuration (ex: `% Test`) is broken, see example below
 
```
  "net.javacrumbs.json-unit"    % "json-unit"               % "2.22.0"   % Test,
- "org.mockito"                 % "mockito-core"            % "3.6.28"   % Test,
+ "org.mockito"                 % "mockito-core"            % "3.7.7"   % Test,
  "org.scalatest"              %% "scalatest"               % "3.2.3"    % Test,
```

Are there any tips/tricks to avoid or workaround this issue? A quick fix is to run scalafmt to fix the alignment but currently scalafmt runs only if there is a scalafmt update. Thoughts?


For reference, scalafmt configuration for aligning library dependencies
```
// Align *.sbt dependencies
align.tokens = [
  {code = "%", owner = "Term.ApplyInfix"},
  {code = "%%", owner = "Term.ApplyInfix"}
]
```

Pull request:
This runs `scalafmt --mode changed` after bumping the version but before
creating the update commit. With `--mode changed` only changed files are
reformatted. This reformatting is different to the `PostUpdateHook` that
runs `scalafmt` on scalafmt updates because formatting changes are not
recorded in a separate commit. This is not (yet) configurable via
`.scala-steward.conf` but that could be added later if needed.

We're also only running scalafmt if the project depends on it.

Example PR: https://github.com/scala-steward-org/test-repo-2/pull/59

Closes: #1889

Patch:
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 005283eb4d..0362750e67 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
@@ -27,7 +27,9 @@ import org.scalasteward.core.edit.scalafix.{ScalafixMigration, ScalafixMigration
 import org.scalasteward.core.git
 import org.scalasteward.core.git.GitAlg
 import org.scalasteward.core.io.{isSourceFile, FileAlg, WorkspaceAlg}
+import org.scalasteward.core.repocache.RepoCache
 import org.scalasteward.core.repoconfig.RepoConfig
+import org.scalasteward.core.scalafmt.{scalafmtModule, ScalafmtAlg}
 import org.scalasteward.core.util._
 import org.scalasteward.core.util.logger._
 import org.scalasteward.core.vcs.data.Repo
@@ -40,6 +42,7 @@ final class EditAlg[F[_]](implicit
     hookExecutor: HookExecutor[F],
     logger: Logger[F],
     scalafixMigrationsFinder: ScalafixMigrationsFinder,
+    scalafmtAlg: ScalafmtAlg[F],
     workspaceAlg: WorkspaceAlg[F],
     F: Concurrent[F]
 ) {
@@ -65,6 +68,7 @@ final class EditAlg[F[_]](implicit
                   gitAlg.discardChanges(repo) *>
                     runScalafixMigrations(repo, data.config, migrations) <*
                     bumpVersion(update, files)
+              _ <- reformatChangedFiles(repo, data.cache)
               updateCommitMsg = git.commitMsgFor(update, data.config.commits, data.repo.branch)
               updateEdit <- gitAlg
                 .commitAllIfDirty(repo, updateCommitMsg)
@@ -111,4 +115,11 @@ final class EditAlg[F[_]](implicit
     }
     bindUntilTrue[Nel, F](actions)
   }
+
+  private def reformatChangedFiles(repo: Repo, cache: RepoCache): F[Unit] =
+    if (cache.dependsOn(List(scalafmtModule)))
+      logger.attemptWarn.log_("Reformatting changed files failed") {
+        scalafmtAlg.reformatChanged(repo)
+      }
+    else F.unit
 }
diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/hooks/HookExecutor.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/hooks/HookExecutor.scala
index 7d00b48db8..263e501fb7 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/edit/hooks/HookExecutor.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/edit/hooks/HookExecutor.scala
@@ -25,6 +25,7 @@ import org.scalasteward.core.edit.EditAttempt.HookEdit
 import org.scalasteward.core.git.{CommitMsg, GitAlg}
 import org.scalasteward.core.io.{ProcessAlg, WorkspaceAlg}
 import org.scalasteward.core.repocache.RepoCache
+import org.scalasteward.core.scalafmt.ScalafmtAlg.opts
 import org.scalasteward.core.scalafmt.{scalafmtArtifactId, scalafmtBinary, scalafmtGroupId}
 import org.scalasteward.core.util.Nel
 import org.scalasteward.core.util.logger._
@@ -75,12 +76,6 @@ object HookExecutor {
   private val conditionalSbtGitHubActionsModules =
     (sbtGroupId, sbtArtifactId) :: scalaLangModules
 
-  private def dependsOnSbtGithubActions(cache: RepoCache): Boolean =
-    cache.dependencyInfos.exists(_.value.exists { info =>
-      val module = (info.dependency.groupId, info.dependency.artifactId)
-      sbtGitHubActionsModules.contains(module)
-    })
-
   private def sbtGithubActionsHook(
       groupId: GroupId,
       artifactId: ArtifactId,
@@ -100,7 +95,7 @@ object HookExecutor {
     PostUpdateHook(
       groupId = scalafmtGroupId,
       artifactId = scalafmtArtifactId,
-      command = Nel.of(scalafmtBinary, "--non-interactive"),
+      command = Nel.of(scalafmtBinary, opts.nonInteractive),
       useSandbox = false,
       commitMessage = update => CommitMsg(s"Reformat with scalafmt ${update.nextVersion}"),
       enabledByCache = _ => true,
@@ -110,6 +105,6 @@ object HookExecutor {
         sbtGithubActionsHook(gid, aid, _ => true)
       } ++
       conditionalSbtGitHubActionsModules.map { case (gid, aid) =>
-        sbtGithubActionsHook(gid, aid, dependsOnSbtGithubActions)
+        sbtGithubActionsHook(gid, aid, _.dependsOn(sbtGitHubActionsModules))
       }
 }
diff --git a/modules/core/src/main/scala/org/scalasteward/core/repocache/RepoCache.scala b/modules/core/src/main/scala/org/scalasteward/core/repocache/RepoCache.scala
index 74df4a231e..2556d82efc 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/repocache/RepoCache.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/repocache/RepoCache.scala
@@ -16,9 +16,10 @@
 
 package org.scalasteward.core.repocache
 
+import cats.syntax.all._
 import io.circe.Codec
 import io.circe.generic.semiauto._
-import org.scalasteward.core.data.{DependencyInfo, Scope}
+import org.scalasteward.core.data.{ArtifactId, DependencyInfo, GroupId, Scope}
 import org.scalasteward.core.git.Sha1
 import org.scalasteward.core.repoconfig.RepoConfig
 
@@ -26,7 +27,15 @@ final case class RepoCache(
     sha1: Sha1,
     dependencyInfos: List[Scope[List[DependencyInfo]]],
     maybeRepoConfig: Option[RepoConfig]
-)
+) {
+  def dependsOn(modules: List[(GroupId, ArtifactId)]): Boolean =
+    dependencyInfos.exists(_.value.exists { info =>
+      modules.exists { case (groupId, artifactId) =>
+        info.dependency.groupId === groupId &&
+          info.dependency.artifactId.name === artifactId.name
+      }
+    })
+}
 
 object RepoCache {
   implicit val repoCacheCodec: Codec[RepoCache] =
diff --git a/modules/core/src/main/scala/org/scalasteward/core/scalafmt/ScalafmtAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/scalafmt/ScalafmtAlg.scala
index 66fdf3c69d..5928702453 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/scalafmt/ScalafmtAlg.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/scalafmt/ScalafmtAlg.scala
@@ -23,9 +23,9 @@ import io.circe.ParsingFailure
 import org.scalasteward.core.application.Config
 import org.scalasteward.core.data.{Scope, Version}
 import org.scalasteward.core.io.{FileAlg, ProcessAlg, WorkspaceAlg}
-import org.scalasteward.core.scalafmt.ScalafmtAlg.parseScalafmtConf
+import org.scalasteward.core.scalafmt.ScalafmtAlg.{opts, parseScalafmtConf}
 import org.scalasteward.core.util.Nel
-import org.scalasteward.core.vcs.data.BuildRoot
+import org.scalasteward.core.vcs.data.{BuildRoot, Repo}
 import org.typelevel.log4cats.Logger
 
 final class ScalafmtAlg[F[_]](config: Config)(implicit
@@ -51,13 +51,24 @@ final class ScalafmtAlg[F[_]](config: Config)(implicit
       .map(version => Scope(List(scalafmtDependency(version)), List(config.defaultResolver)))
       .value
 
-  def version: F[String] =
-    workspaceAlg.rootDir
-      .flatMap(processAlg.exec(Nel.of(scalafmtBinary, "--version"), _))
-      .map(_.mkString.trim)
+  def reformatChanged(repo: Repo): F[Unit] = {
+    val cmd = Nel.of(scalafmtBinary, opts.nonInteractive) ++ opts.modeChanged
+    workspaceAlg.repoDir(repo).flatMap(processAlg.exec(cmd, _)).void
+  }
+
+  def version: F[String] = {
+    val cmd = Nel.of(scalafmtBinary, opts.version)
+    workspaceAlg.rootDir.flatMap(processAlg.exec(cmd, _)).map(_.mkString.trim)
+  }
 }
 
 object ScalafmtAlg {
+  object opts {
+    val modeChanged = List("--mode", "changed")
+    val nonInteractive = "--non-interactive"
+    val version = "--version"
+  }
+
   private[scalafmt] def parseScalafmtConf(s: String): Either[ParsingFailure, Option[Version]] =
     io.circe.config.parser.parse(s).map {
       _.asObject.flatMap(_.apply("version")).flatMap(_.asString).map(Version.apply)
diff --git a/modules/core/src/main/scala/org/scalasteward/core/scalafmt/package.scala b/modules/core/src/main/scala/org/scalasteward/core/scalafmt/package.scala
index 5079f7c0bc..ec38a72eb3 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/scalafmt/package.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/scalafmt/package.scala
@@ -29,6 +29,9 @@ package object scalafmt {
     ArtifactId(core, s"${core}_$defaultScalaBinaryVersion")
   }
 
+  val scalafmtModule: (GroupId, ArtifactId) =
+    (scalafmtGroupId, scalafmtArtifactId)
+
   def isScalafmtUpdate(update: Update.Single): Boolean =
     update.groupId === scalafmtGroupId && update.artifactId.name === scalafmtArtifactId.name
 
@@ -40,5 +43,5 @@ package object scalafmt {
 
   val scalafmtBinary: String = "scalafmt"
 
-  val scalafmtConfName = ".scalafmt.conf"
+  val scalafmtConfName: String = ".scalafmt.conf"
 }


Test patch:
diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala
index e6eefb4aac..db38d19013 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala
@@ -5,13 +5,14 @@ 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.{GroupId, RepoData, Update}
+import org.scalasteward.core.data._
 import org.scalasteward.core.mock.MockConfig.{config, gitCmd}
 import org.scalasteward.core.mock.MockContext.context.editAlg
 import org.scalasteward.core.mock.MockState
 import org.scalasteward.core.mock.MockState.TraceEntry.{Cmd, Log}
 import org.scalasteward.core.repoconfig.RepoConfig
-import org.scalasteward.core.scalafmt.{scalafmtBinary, scalafmtConfName}
+import org.scalasteward.core.scalafmt.ScalafmtAlg.opts
+import org.scalasteward.core.scalafmt.{scalafmtBinary, scalafmtConfName, scalafmtDependency}
 import org.scalasteward.core.util.Nel
 import org.scalasteward.core.vcs.data.Repo
 
@@ -57,7 +58,10 @@ class EditAlgTest extends FunSuite {
 
   test("applyUpdate with scalafmt update") {
     val repo = Repo("edit-alg", "test-2")
-    val data = RepoData(repo, dummyRepoCache, RepoConfig.empty)
+    val cache = dummyRepoCache.copy(dependencyInfos =
+      List(List(DependencyInfo(scalafmtDependency(Version("2.0.0")), Nil)).withMavenCentral)
+    )
+    val data = RepoData(repo, cache, RepoConfig.empty)
     val repoDir = config.workspace / repo.toPath
     val update = Update.Single("org.scalameta" % "scalafmt-core" % "2.0.0", Nel.of("2.1.0"))
     val scalafmtConf = repoDir / scalafmtConfName
@@ -95,9 +99,12 @@ class EditAlgTest extends FunSuite {
         Log("Trying heuristic 'specific'"),
         Cmd("read", scalafmtConf.pathAsString),
         Cmd("write", scalafmtConf.pathAsString),
+        Cmd(
+          "VAR1=val1" :: "VAR2=val2" :: repoDir.toString :: scalafmtBinary :: opts.nonInteractive :: opts.modeChanged
+        ),
         Cmd(gitStatus(repoDir)),
         Log("Executing post-update hook for org.scalameta:scalafmt-core"),
-        Cmd("VAR1=val1", "VAR2=val2", repoDir.toString, scalafmtBinary, "--non-interactive"),
+        Cmd("VAR1=val1", "VAR2=val2", repoDir.toString, scalafmtBinary, opts.nonInteractive),
         Cmd(gitStatus(repoDir))
       ),
       files = Map(
diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/hooks/HookExecutorTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/hooks/HookExecutorTest.scala
index 37f204177b..904cee84af 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/edit/hooks/HookExecutorTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/edit/hooks/HookExecutorTest.scala
@@ -10,6 +10,7 @@ import org.scalasteward.core.mock.MockContext.context.{hookExecutor, workspaceAl
 import org.scalasteward.core.mock.MockState
 import org.scalasteward.core.mock.MockState.TraceEntry.{Cmd, Log}
 import org.scalasteward.core.repoconfig.{RepoConfig, ScalafmtConfig}
+import org.scalasteward.core.scalafmt.ScalafmtAlg.opts
 import org.scalasteward.core.scalafmt.{scalafmtArtifactId, scalafmtBinary, scalafmtGroupId}
 import org.scalasteward.core.util.Nel
 import org.scalasteward.core.vcs.data.Repo
@@ -40,7 +41,7 @@ class HookExecutorTest extends CatsEffectSuite {
     val expected = initial.copy(
       trace = Vector(
         Log("Executing post-update hook for org.scalameta:scalafmt-core"),
-        Cmd("VAR1=val1", "VAR2=val2", repoDir.toString, scalafmtBinary, "--non-interactive"),
+        Cmd("VAR1=val1", "VAR2=val2", repoDir.toString, scalafmtBinary, opts.nonInteractive),
         Cmd(
           gitCmd(repoDir),
           "status",