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 the CLI option to fetch only recently changed files for formating. #1416

Merged
merged 6 commits into from May 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/_scalafmt
Expand Up @@ -16,7 +16,7 @@ _scalafmt() {
'--assume-filename[when using --stdin, use --assume-filename to hint to scalafmt that the input is an .sbt file]:value:' \
'--test[test for mis-formatted code, exits with status 1 on failure]' \
'--migrate2hocon[migrate .scalafmt CLI style configuration to hocon style configuration in .scalafmt.conf]:value:' \
'--diff[if set, only format edited files in git diff against master]' \
'--mode diff[if set, only format edited files in git diff against master]' \
'--diff-branch[if set, only format edited files in git diff against provided branch]:value:' \
'--build-info[prints build information]' \
'--quiet[do not print out stuff to console]' \
Expand Down
20 changes: 16 additions & 4 deletions scalafmt-cli/src/main/scala/org/scalafmt/cli/CliArgParser.scala
Expand Up @@ -15,7 +15,8 @@ object CliArgParser {
| # 2. .scalafmt.conf inside root directory of current git repo
| # 3. no configuration, default style
|scalafmt --test # throw exception on mis-formatted files, won't write to files.
|scalafmt --diff # Format all files that were edited in git diff against master branch.
|scalafmt --mode diff # Format all files that were edited in git diff against master branch.
|scalafmt --mode changed # Format files listed in `git status` (latest changes against previous commit.
|scalafmt --diff-branch 2.x # same as --diff, except against branch 2.x
|scalafmt --stdin # read from stdin and print to stdout
|scalafmt --stdin --assume-filename foo.sbt < foo.sbt # required when using --stdin to format .sbt files.
Expand Down Expand Up @@ -135,12 +136,23 @@ object CliArgParser {
"""migrate .scalafmt CLI style configuration to hocon style configuration in .scalafmt.conf"""
)
opt[Unit]("diff")
.action((_, c) => c.copy(diff = Some("master")))
.text("If set, only format edited files in git diff against master.")
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep --diff option available (as an alias of --mode diff) to avoid breaking changes?
In that case, the description of --diff would be sufficient with something like Alias to "--mode diff".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Makes sense.

.action((_, c) => c.copy(mode = Option(DiffFiles("master"))))
.text(
s"""Format files listed in `git diff` against master.
|Deprecated: use --mode diff instead""".stripMargin
)
opt[FileFetchMode]("mode")
.action((m, c) => c.copy(mode = Option(m)))
.text(
s"""Sets the files to be formatted fetching mode.
|Options:
| diff - format files listed in `git diff` against master
| changed - format files listed in `git status` (latest changes against previous commit)""".stripMargin
)
opt[String]("diff-branch")
.action((branch, c) => c.copy(diff = Some(branch)))
.text(
"If set, only format edited files in git diff against provided branch."
"If set, only format edited files in git diff against provided branch. Has no effect if mode set to `changed`."
Copy link
Member

Choose a reason for hiding this comment

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

👍

)
opt[Unit]("build-info")
.action({
Expand Down
9 changes: 4 additions & 5 deletions scalafmt-cli/src/main/scala/org/scalafmt/cli/CliOptions.scala
Expand Up @@ -45,6 +45,7 @@ object CliOptions {
config = style
)
}

private def getConfigJFile(file: AbsoluteFile): AbsoluteFile =
file / ".scalafmt.conf"

Expand Down Expand Up @@ -85,6 +86,7 @@ case class CliOptions(
debug: Boolean = false,
git: Option[Boolean] = None,
nonInteractive: Boolean = false,
mode: Option[FileFetchMode] = None,
diff: Option[String] = None,
assumeFilename: String = "stdin.scala", // used when read from stdin
migrate: Option[AbsoluteFile] = None,
Expand Down Expand Up @@ -145,11 +147,8 @@ case class CliOptions(

val inPlace: Boolean = writeMode == Override

val fileFetchMode: FileFetchMode = {
diff.map(DiffFiles).getOrElse {
if (isGit) GitFiles else RecursiveSearch
}
}
val fileFetchMode: FileFetchMode =
mode.orElse(Some(GitFiles).filter(_ => isGit)).getOrElse(RecursiveSearch)

val files: Seq[AbsoluteFile] =
if (customFiles.isEmpty)
Expand Down
21 changes: 21 additions & 0 deletions scalafmt-cli/src/main/scala/org/scalafmt/cli/FileFetchMode.scala
@@ -1,10 +1,24 @@
package org.scalafmt.cli

import scopt.Read

/**
* Determines how we fetch files for formatting
*/
sealed trait FileFetchMode

object FileFetchMode {

/**
* The read instance is practically is not exhaustive due to the RecursiveSearch and GitFiles are the fallback used in the absence of
* other options
*/
implicit val read: Read[FileFetchMode] = Read.reads {
case "diff" => DiffFiles("master")
case "changed" => ChangedFiles
}
}

/**
* A simple recursive strategy where each directory is expanded
*/
Expand All @@ -21,3 +35,10 @@ final case object GitFiles extends FileFetchMode
* When this is set, files passed via the cli are ignored.
*/
final case class DiffFiles(branch: String) extends FileFetchMode

/**
* A call to `git status --porcelain`
*
* When this is set, files passed via the cli are ignored.
*/
final case object ChangedFiles extends FileFetchMode
Expand Up @@ -32,7 +32,7 @@ object ScalafmtCoreRunner extends ScalafmtRunner {
)

val inputMethods = getInputMethods(options, Some(filterMatcher))
if (inputMethods.isEmpty && options.diff.isEmpty && !options.stdIn)
if (inputMethods.isEmpty && options.mode.isEmpty && !options.stdIn)
throw NoMatchingFiles

val counter = new AtomicInteger()
Expand Down
Expand Up @@ -14,7 +14,7 @@ object ScalafmtDynamicRunner extends ScalafmtRunner {
termDisplayMessage: String
): ExitCode = {
val inputMethods = getInputMethods(options, None)
if (inputMethods.isEmpty && options.diff.isEmpty && !options.stdIn)
if (inputMethods.isEmpty && options.mode.isEmpty && !options.stdIn)
throw NoMatchingFiles

val counter = new AtomicInteger()
Expand Down
Expand Up @@ -50,7 +50,8 @@ trait ScalafmtRunner {
filter: Option[FilterMatcher]
): Seq[AbsoluteFile] = {
def canFormat(f: AbsoluteFile): Boolean =
filter.map(_.matches(f)).getOrElse(true)
filter.forall(_.matches(f))

val files = options.fileFetchMode match {
case m @ (GitFiles | RecursiveSearch) =>
val fetchFiles: AbsoluteFile => Seq[AbsoluteFile] =
Expand All @@ -63,8 +64,12 @@ trait ScalafmtRunner {
// formatted regardless of what it is or where it is.
case f => Seq(f)
}

case DiffFiles(branch) =>
options.gitOps.diff(branch).filter(canFormat)

case ChangedFiles =>
options.gitOps.status.filter(canFormat)
}
val excludeRegexp = options.excludeFilterRegexp
files.filter { f =>
Expand Down
Expand Up @@ -5,6 +5,7 @@ import java.io.File
/** Wrapper around java.io.File with an absolute path. */
sealed abstract case class AbsoluteFile(jfile: File) {
def path: String = jfile.getAbsolutePath
def exists: Boolean = jfile.exists()
def /(other: String) = new AbsoluteFile(new File(jfile, other)) {}

override def toString(): String = path
Expand Down
49 changes: 45 additions & 4 deletions scalafmt-core/shared/src/main/scala/org/scalafmt/util/GitOps.scala
@@ -1,12 +1,12 @@
package org.scalafmt.util

import scala.sys.process.ProcessLogger
import scala.util.Try
import scala.util.{Failure, Success, Try}
import scala.util.control.NonFatal

import java.io.File

trait GitOps {
def status: Seq[AbsoluteFile]
def diff(branch: String): Seq[AbsoluteFile]
def lsTree(dir: AbsoluteFile): Seq[AbsoluteFile]
def rootDir: Option[AbsoluteFile]
Expand Down Expand Up @@ -50,7 +50,7 @@ class GitOpsImpl(private[util] val workingDirectory: AbsoluteFile)
"--full-name",
dir.path
)
).toOption.toSeq.flatten.map(f => rtDir / f)
).getOrElse(Seq.empty).map(f => rtDir / f)
}

override def rootDir: Option[AbsoluteFile] = {
Expand All @@ -76,7 +76,48 @@ class GitOpsImpl(private[util] val workingDirectory: AbsoluteFile)
)
for {
root <- rootDir.toSeq
path <- exec(cmd).toOption.toSeq.flatten
path <- exec(cmd).getOrElse(Seq.empty)
} yield AbsoluteFile.fromFile(new File(path), root)
}

override def status: Seq[AbsoluteFile] = {
val cmd = Seq(
"git",
"status",
"--porcelain"
)
for {
root <- rootDir.toSeq
statusLine <- exec(cmd).getOrElse(Seq.empty)
file <- getFileFromGitStatusLine(statusLine).toOption.toSeq
} yield AbsoluteFile.fromFile(file, root)
}.filter(_.exists)

private final val renameStatusCode = "R"
private final val renameStatusArrowDelimiter = "-> "

/*
Method extracts path to changed file from the singular line of the `git status --porcelain` output.
(see https://git-scm.com/docs/git-status#_short_format)
*/
private def extractPathPart(s: String): Try[String] =
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some comments that explain the behavior (and include a reference to https://git-scm.com/docs/git-status#_short_format)?
Now it's a bit complicated and worth explaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 👍

Try(
Option(s)
// Checks if the line status states the file was renamed (E.g: `R ORIG_PATH -> PATH`)
.filter(_.substring(0, 2).contains(renameStatusCode))
// takes the part of the string after the `-> ` character sequence
.map(_.split(renameStatusArrowDelimiter).last)
// fallback for the regular status line (E.g.: `XY PATH`)
// Drops the status codes by splitting on white spaces then taking the tail of the result
// Restores spaces in the path by merging the tail back with white space separator
.getOrElse(s.trim.split(' ').tail.mkString(" "))
.trim
)

private def trimQuotes(s: String): String =
s.replaceAll("^\"|\"$", "")

private def getFileFromGitStatusLine(s: String): Try[File] =
extractPathPart(s)
.map(pathRaw => new File(trimQuotes(pathRaw)))
}
11 changes: 7 additions & 4 deletions scalafmt-tests/src/test/scala/org/scalafmt/cli/CliTest.scala
Expand Up @@ -71,6 +71,7 @@ abstract class AbstractCliTest extends FunSuite with DiffAssertions {

def gimmeConfig(string: String): ScalafmtConfig =
Config.fromHoconString(string).get

def noArgTest(
input: AbsoluteFile,
expected: String,
Expand Down Expand Up @@ -314,11 +315,12 @@ trait CliTestBehavior { this: AbstractCliTest =>
}

test(
s"scalafmt (no matching files) is okay with --diff and --stdin: $label"
s"scalafmt (no matching files) is okay with --mode diff and --stdin: $label"
) {
val diff = getConfig(
Array(
"--diff",
"--mode",
"diff",
"--config-str",
s"""{version="$version",style=IntelliJ}"""
)
Expand Down Expand Up @@ -370,9 +372,10 @@ trait CliTestBehavior { this: AbstractCliTest =>
noArgTest(
input,
expected,
Seq(Array.empty[String], Array("--diff"))
Seq(Array.empty[String], Array("--mode", "diff"))
)
}

test(s"scalafmt (no arg, no config): $label") {
noArgTest(
string2dir(
Expand Down Expand Up @@ -459,7 +462,7 @@ trait CliTestBehavior { this: AbstractCliTest =>
}

test(
s"includeFilters are ignored for full paths but NOT ignore for passed directories: $label"
s"includeFilters are ignored for full paths but NOT test for passed directories: $label"
) {
val root =
string2dir(
Expand Down
Expand Up @@ -8,5 +8,6 @@ class FakeGitOps(root: AbsoluteFile) extends GitOps {
override def lsTree(dir: AbsoluteFile): Vector[AbsoluteFile] =
FileOps.listFiles(dir)
override def rootDir: Option[AbsoluteFile] = Some(root)
override def status: Seq[AbsoluteFile] = lsTree(root)
override def diff(branch: String): Seq[AbsoluteFile] = lsTree(root)
}