From b44c58b410d2a871bf71ec950f76d194c01cfbb3 Mon Sep 17 00:00:00 2001 From: Trevor Debrechtabel Date: Thu, 30 Sep 2021 17:42:31 -0700 Subject: [PATCH] Added dry run mode (#212) Summary: In --dry-run mode, ktfmt will print the file paths of files that need formatting but will not change the file contents. Also added the option to exit the program with code `1` if any changes are needed rather than just if any errors happened. These changes are in alignment with google-java-format's identical options (see https://github.com/google/google-java-format/pull/106). Usage: --dry-run or -n Prints the paths of the files whose contents would change if the formatter were run normally. --set-exit-if-changed Return exit code 1 if there are any formatting changes made (or detected if running in dry mode). Reviewed By: cgrushko Differential Revision: D31286652 fbshipit-source-id: c89fb72a1411766b81c28eba66614df78b0fea0a --- core/src/main/java/com/facebook/ktfmt/Main.kt | 62 ++++-- .../java/com/facebook/ktfmt/ParsedArgs.kt | 20 +- .../java/com/facebook/ktfmt/MainKtTest.kt | 204 ++++++++++++++---- .../java/com/facebook/ktfmt/ParsedArgsTest.kt | 39 +++- 4 files changed, 262 insertions(+), 63 deletions(-) diff --git a/core/src/main/java/com/facebook/ktfmt/Main.kt b/core/src/main/java/com/facebook/ktfmt/Main.kt index f5845835..db5301ed 100644 --- a/core/src/main/java/com/facebook/ktfmt/Main.kt +++ b/core/src/main/java/com/facebook/ktfmt/Main.kt @@ -16,7 +16,6 @@ package com.facebook.ktfmt -import com.google.common.annotations.VisibleForTesting import com.google.googlejavaformat.FormattingError import java.io.BufferedReader import java.io.File @@ -47,7 +46,7 @@ class Main( } if (parsedArgs.fileNames.size == 1 && parsedArgs.fileNames[0] == "-") { - val success = formatStdin() + val success = format(null) return if (success) 0 else 1 } @@ -65,36 +64,53 @@ class Main( } val success = AtomicBoolean(true) - files.parallelStream().forEach { success.compareAndSet(true, formatFile(it)) } + files.parallelStream().forEach { success.compareAndSet(true, format(it)) } return if (success.get()) 0 else 1 } - @VisibleForTesting - fun formatStdin(): Boolean { - val code = BufferedReader(InputStreamReader(input)).readText() + /** + * Handles the logic for formatting and flags. + * + * If dry run mode is active, this simply prints the name of the [source] (file path or ``) + * to [out]. Otherwise, this will run the appropriate formatting as normal. + * + * @param file The file to format. If null, the code is read from . + * @return True if changes were made or no changes were needed, false if there was a failure or if + * changes were made and the --set-exit-if-changed flag is set. + */ + private fun format(file: File?): Boolean { + val fileName = file?.toString() ?: "" try { - out.print(format(parsedArgs.formattingOptions, code)) - return true - } catch (e: ParseError) { - handleParseError("", e) - } - return false - } + val code = file?.readText() ?: BufferedReader(InputStreamReader(input)).readText() - /** 'formatFile' formats 'file' in place, and return whether it was successful. */ - private fun formatFile(file: File): Boolean { - try { - val code = file.readText() - file.writeText(format(parsedArgs.formattingOptions, code)) - err.println("Done formatting $file") - return true + val formattedCode = format(parsedArgs.formattingOptions, code) + + if (code == formattedCode) { + // The code was already formatted, nothing more to do here + err.println("No changes: $fileName") + return true + } + + if (parsedArgs.dryRun) { + out.println(fileName) + } else { + if (file != null) { + file.writeText(formattedCode) + } else { + out.print(formattedCode) + } + err.println("Done formatting $fileName") + } + + // If setExitIfChanged is true, then any change should result in an exit code of 1. + return if (parsedArgs.setExitIfChanged) false else true } catch (e: IOException) { - err.println("Error formatting $file: ${e.message}; skipping.") + err.println("Error formatting $fileName: ${e.message}; skipping.") } catch (e: ParseError) { - handleParseError(file.toString(), e) + handleParseError(fileName, e) } catch (e: FormattingError) { for (diagnostic in e.diagnostics()) { - System.err.println("$file:$diagnostic") + System.err.println("$fileName:$diagnostic") } e.printStackTrace(err) } diff --git a/core/src/main/java/com/facebook/ktfmt/ParsedArgs.kt b/core/src/main/java/com/facebook/ktfmt/ParsedArgs.kt index 61a907df..aa638016 100644 --- a/core/src/main/java/com/facebook/ktfmt/ParsedArgs.kt +++ b/core/src/main/java/com/facebook/ktfmt/ParsedArgs.kt @@ -19,20 +19,36 @@ package com.facebook.ktfmt import java.io.PrintStream /** ParsedArgs holds the arguments passed to ktfmt on the command-line, after parsing. */ -data class ParsedArgs(val fileNames: List, val formattingOptions: FormattingOptions) +data class ParsedArgs( + val fileNames: List, + val formattingOptions: FormattingOptions, + /** + * Run the formatter without writing changes to any files. This will print the path of any files + * that would be changed if the formatter is run normally. + */ + val dryRun: Boolean, + + /** Return exit code 1 if any formatting changes are detected. */ + val setExitIfChanged: Boolean, +) /** parseOptions parses command-line arguments passed to ktfmt. */ fun parseOptions(err: PrintStream, args: Array): ParsedArgs { val fileNames = mutableListOf() var formattingOptions = FormattingOptions() + var dryRun = false + var setExitIfChanged = false + for (arg in args) { when { arg == "--dropbox-style" -> formattingOptions = DROPBOX_FORMAT arg == "--google-style" -> formattingOptions = GOOGLE_FORMAT arg == "--kotlinlang-style" -> formattingOptions = KOTLINLANG_FORMAT + arg == "--dry-run" || arg == "-n" -> dryRun = true + arg == "--set-exit-if-changed" -> setExitIfChanged = true arg.startsWith("--") -> err.println("Unexpected option: $arg") else -> fileNames.add(arg) } } - return ParsedArgs(fileNames, formattingOptions) + return ParsedArgs(fileNames, formattingOptions, dryRun, setExitIfChanged) } diff --git a/core/src/test/java/com/facebook/ktfmt/MainKtTest.kt b/core/src/test/java/com/facebook/ktfmt/MainKtTest.kt index c9e3c72e..c83234ce 100644 --- a/core/src/test/java/com/facebook/ktfmt/MainKtTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/MainKtTest.kt @@ -34,6 +34,10 @@ class MainKtTest { private val root = createTempDir() + private val emptyInput = "".byteInputStream() + private val out = ByteArrayOutputStream() + private val err = ByteArrayOutputStream() + @After fun tearDown() { root.deleteRecursively() @@ -98,29 +102,22 @@ class MainKtTest { } @Test - fun `formatStdin formats an InputStream`() { + fun `Using '-' as the filename formats an InputStream`() { val code = "fun f1 ( ) : Int = 0" - val output = ByteArrayOutputStream() - Main(code.byteInputStream(), PrintStream(output), PrintStream(output), arrayOf()).formatStdin() + Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("-")).run() val expected = """fun f1(): Int = 0 |""".trimMargin() - assertThat(output.toString("UTF-8")).isEqualTo(expected) + assertThat(out.toString("UTF-8")).isEqualTo(expected) } @Test fun `Parsing errors are reported (stdin)`() { val code = "fun f1 ( " - val err = ByteArrayOutputStream() val returnValue = - Main( - code.byteInputStream(), - PrintStream(ByteArrayOutputStream()), - PrintStream(err), - arrayOf()) - .formatStdin() + Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("-")).run() - assertThat(returnValue).isFalse() + assertThat(returnValue).isEqualTo(1) assertThat(err.toString("UTF-8")).startsWith(":1:14: error: ") } @@ -128,14 +125,8 @@ class MainKtTest { fun `Parsing errors are reported (file)`() { val fooBar = root.resolve("foo.kt") fooBar.writeText("fun f1 ( ") - val err = ByteArrayOutputStream() val returnValue = - Main( - "".byteInputStream(), - PrintStream(ByteArrayOutputStream()), - PrintStream(err), - arrayOf(fooBar.toString())) - .run() + Main(emptyInput, PrintStream(out), PrintStream(err), arrayOf(fooBar.toString())).run() assertThat(returnValue).isEqualTo(1) assertThat(err.toString("UTF-8")).contains("foo.kt:1:14: error: ") @@ -149,7 +140,6 @@ class MainKtTest { file1.writeText("fun f1 () ") file2Broken.writeText("fun f1 ( ") file3.writeText("fun f1 () ") - val err = ByteArrayOutputStream() // Make Main() process files serially. val forkJoinPool = ForkJoinPool(1) @@ -158,8 +148,8 @@ class MainKtTest { forkJoinPool .submit { Main( - "".byteInputStream(), - PrintStream(ByteArrayOutputStream()), + emptyInput, + PrintStream(out), PrintStream(err), arrayOf(file1.toString(), file2Broken.toString(), file3.toString())) .run() @@ -185,11 +175,10 @@ class MainKtTest { val fooBar = root.resolve("foo.kt") fooBar.writeText(code) - val output = ByteArrayOutputStream() Main( - "".byteInputStream(), - PrintStream(output), - PrintStream(output), + emptyInput, + PrintStream(out), + PrintStream(err), arrayOf("--dropbox-style", fooBar.toString())) .run() @@ -200,21 +189,28 @@ class MainKtTest { fun `dropbox-style is passed to formatter (stdin)`() { val code = """fun f() { - for (child in - node.next.next.next.next.next.next.next.next.next.next.next.next.next.next.data()) { - println(child) - } -} -""" - val output = ByteArrayOutputStream() + |for (child in + |node.next.next.next.next.next.next.next.next.next.next.next.next.next.next.data()) { + |println(child) + |} + |} + |""".trimMargin() + val formatted = + """fun f() { + | for (child in + | node.next.next.next.next.next.next.next.next.next.next.next.next.next.next.data()) { + | println(child) + | } + |} + |""".trimMargin() Main( code.byteInputStream(), - PrintStream(output), - PrintStream(output), + PrintStream(out), + PrintStream(err), arrayOf("--dropbox-style", "-")) .run() - assertThat(output.toString("UTF-8")).isEqualTo(code) + assertThat(out.toString("UTF-8")).isEqualTo(formatted) } @Test @@ -237,4 +233,140 @@ class MainKtTest { assertThat(expandArgsToFileNames(files.map { it.toString() })) .containsExactly(f1, f2, f5, f6, f7) } + + @Test + fun `--dry-run prints filename and does not change file`() { + val code = """fun f () = println( "hello, world" )""" + val file = root.resolve("foo.kt") + file.writeText(code) + + Main(emptyInput, PrintStream(out), PrintStream(err), arrayOf("--dry-run", file.toString())) + .run() + + assertThat(file.readText()).isEqualTo(code) + assertThat(out.toString("UTF-8")).contains(file.toString()) + } + + @Test + fun `--dry-run prints 'stdin' and does not reformat code from stdin`() { + val code = """fun f () = println( "hello, world" )""" + + Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("--dry-run", "-")) + .run() + + assertThat(out.toString("UTF-8")).doesNotContain("hello, world") + assertThat(out.toString("UTF-8")).isEqualTo("\n") + } + + @Test + fun `--dry-run prints nothing when there are no changes needed (file)`() { + val code = """fun f() = println("hello, world")\n""" + val file = root.resolve("foo.kt") + file.writeText(code) + + Main(emptyInput, PrintStream(out), PrintStream(err), arrayOf("--dry-run", file.toString())) + .run() + + assertThat(out.toString("UTF-8")).isEmpty() + } + + @Test + fun `--dry-run prints nothing when there are no changes needed (stdin)`() { + val code = """fun f() = println("hello, world")\n""" + + Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("--dry-run", "-")) + .run() + + assertThat(out.toString("UTF-8")).isEmpty() + } + + @Test + fun `Exit code is 0 when there are changes (file)`() { + val code = """fun f () = println( "hello, world" )""" + val file = root.resolve("foo.kt") + file.writeText(code) + + val exitCode = + Main(emptyInput, PrintStream(out), PrintStream(err), arrayOf(file.toString())).run() + + assertThat(exitCode).isEqualTo(0) + } + + @Test + fun `Exits with 0 when there are changes (stdin)`() { + val code = """fun f () = println( "hello, world" )""" + + val exitCode = + Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("-")).run() + + assertThat(exitCode).isEqualTo(0) + } + + @Test + fun `Exit code is 1 when there are changes and --set-exit-if-changed is set (file)`() { + val code = """fun f () = println( "hello, world" )""" + val file = root.resolve("foo.kt") + file.writeText(code) + + val exitCode = + Main( + emptyInput, + PrintStream(out), + PrintStream(err), + arrayOf("--set-exit-if-changed", file.toString())) + .run() + + assertThat(exitCode).isEqualTo(1) + } + + @Test + fun `Exit code is 1 when there are changes and --set-exit-if-changed is set (stdin)`() { + val code = """fun f () = println( "hello, world" )""" + + val exitCode = + Main( + code.byteInputStream(), + PrintStream(out), + PrintStream(err), + arrayOf("--set-exit-if-changed", "-")) + .run() + + assertThat(exitCode).isEqualTo(1) + } + + @Test + fun `--set-exit-if-changed and --dry-run changes nothing, prints filenames, and exits with 1 (file)`() { + val code = """fun f () = println( "hello, world" )""" + val file = root.resolve("foo.kt") + file.writeText(code) + + val exitCode = + Main( + emptyInput, + PrintStream(out), + PrintStream(err), + arrayOf("--dry-run", "--set-exit-if-changed", file.toString())) + .run() + + assertThat(file.readText()).isEqualTo(code) + assertThat(out.toString("UTF-8")).contains(file.toString()) + assertThat(exitCode).isEqualTo(1) + } + + @Test + fun `--set-exit-if-changed and --dry-run changes nothing, prints filenames, and exits with 1 (stdin)`() { + val code = """fun f () = println( "hello, world" )""" + + val exitCode = + Main( + code.byteInputStream(), + PrintStream(out), + PrintStream(err), + arrayOf("--dry-run", "--set-exit-if-changed", "-")) + .run() + + assertThat(out.toString("UTF-8")).doesNotContain("hello, world") + assertThat(out.toString("UTF-8")).isEqualTo("\n") + assertThat(exitCode).isEqualTo(1) + } } diff --git a/core/src/test/java/com/facebook/ktfmt/ParsedArgsTest.kt b/core/src/test/java/com/facebook/ktfmt/ParsedArgsTest.kt index ae141812..6109a44c 100644 --- a/core/src/test/java/com/facebook/ktfmt/ParsedArgsTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/ParsedArgsTest.kt @@ -38,13 +38,21 @@ class ParsedArgsTest { } @Test - fun `parseOptions returns default indent sizes when --dropbox-style is not present`() { + fun `parseOptions uses default values when args are empty`() { val out = ByteArrayOutputStream() - val (_, formattingOptions) = parseOptions(PrintStream(out), arrayOf("foo.kt")) + val parsed = parseOptions(PrintStream(out), arrayOf("foo.kt")) + val formattingOptions = parsed.formattingOptions + assertThat(formattingOptions.style).isEqualTo(FormattingOptions.Style.FACEBOOK) + assertThat(formattingOptions.maxWidth).isEqualTo(100) assertThat(formattingOptions.blockIndent).isEqualTo(2) assertThat(formattingOptions.continuationIndent).isEqualTo(4) + assertThat(formattingOptions.removeUnusedImports).isTrue() + assertThat(formattingOptions.debuggingPrintOpsAfterFormatting).isFalse() + + assertThat(parsed.dryRun).isFalse() + assertThat(parsed.setExitIfChanged).isFalse() } @Test @@ -68,4 +76,31 @@ class ParsedArgsTest { assertThat(formattingOptions).isEqualTo(GOOGLE_FORMAT) } + + @Test + fun `parseOptions recognizes --dry-run`() { + val out = ByteArrayOutputStream() + + val parsed = parseOptions(PrintStream(out), arrayOf("--dry-run", "foo.kt")) + + assertThat(parsed.dryRun).isTrue() + } + + @Test + fun `parseOptions recognizes -n as --dry-run`() { + val out = ByteArrayOutputStream() + + val parsed = parseOptions(PrintStream(out), arrayOf("-n", "foo.kt")) + + assertThat(parsed.dryRun).isTrue() + } + + @Test + fun `parseOptions recognizes --set-exit-if-changed`() { + val out = ByteArrayOutputStream() + + val parsed = parseOptions(PrintStream(out), arrayOf("--set-exit-if-changed", "foo.kt")) + + assertThat(parsed.setExitIfChanged).isTrue() + } }