From 8defaca8cc3c852c4615e57687577aa41ccfc95c Mon Sep 17 00:00:00 2001 From: Joseph Cooper Date: Mon, 13 May 2024 17:45:16 +0100 Subject: [PATCH 1/3] Refactor CLI argument parsing Closes #465 --- .../main/java/com/facebook/ktfmt/cli/Main.kt | 30 +++-- .../java/com/facebook/ktfmt/cli/ParsedArgs.kt | 35 +++--- .../com/facebook/ktfmt/cli/ParsedArgsTest.kt | 117 ++++++------------ 3 files changed, 79 insertions(+), 103 deletions(-) diff --git a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt index 5f46eb92..3b826966 100644 --- a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt +++ b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt @@ -36,7 +36,7 @@ class Main( private val input: InputStream, private val out: PrintStream, private val err: PrintStream, - args: Array + private val args: Array ) { companion object { @JvmStatic @@ -69,9 +69,14 @@ class Main( } } - private val parsedArgs: ParsedArgs = ParsedArgs.processArgs(err, args) + fun run(): Int { + val processArgs = ParsedArgs.processArgs(args) + val parsedArgs = when (processArgs) { + is ParseResult.Ok -> processArgs.parsedValue + is ParseResult.Error-> exitFatal(processArgs.errorMessage, 1) + } if (parsedArgs.fileNames.isEmpty()) { err.println( "Usage: ktfmt [--dropbox-style | --google-style | --kotlinlang-style] [--dry-run] [--set-exit-if-changed] [--stdin-name=] [--do-not-remove-unused-imports] File1.kt File2.kt ...") @@ -81,7 +86,7 @@ class Main( if (parsedArgs.fileNames.size == 1 && parsedArgs.fileNames[0] == "-") { return try { - val alreadyFormatted = format(null) + val alreadyFormatted = format(null,parsedArgs) if (!alreadyFormatted && parsedArgs.setExitIfChanged) 1 else 0 } catch (e: Exception) { 1 @@ -107,7 +112,7 @@ class Main( val retval = AtomicInteger(0) files.parallelStream().forEach { try { - if (!format(it) && parsedArgs.setExitIfChanged) { + if (!format(it, parsedArgs) && parsedArgs.setExitIfChanged) { retval.set(1) } } catch (e: Exception) { @@ -126,17 +131,17 @@ class Main( * @param file The file to format. If null, the code is read from . * @return true iff input is valid and already formatted. */ - private fun format(file: File?): Boolean { - val fileName = file?.toString() ?: parsedArgs.stdinName ?: "" + private fun format(file: File?, args: ParsedArgs): Boolean { + val fileName = file?.toString() ?: args.stdinName ?: "" try { val bytes = if (file == null) input else FileInputStream(file) val code = BufferedReader(InputStreamReader(bytes, UTF_8)).readText() - val formattedCode = Formatter.format(parsedArgs.formattingOptions, code) + val formattedCode = Formatter.format(args.formattingOptions, code) val alreadyFormatted = code == formattedCode // stdin if (file == null) { - if (parsedArgs.dryRun) { + if (args.dryRun) { if (!alreadyFormatted) { out.println(fileName) } @@ -146,7 +151,7 @@ class Main( return alreadyFormatted } - if (parsedArgs.dryRun) { + if (args.dryRun) { if (!alreadyFormatted) { out.println(fileName) } @@ -173,4 +178,11 @@ class Main( throw e } } + + fun exitFatal(message: String, returnCode: Int): Nothing { + err.println(message) + exitProcess(returnCode) + } + } + diff --git a/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt b/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt index e0009f7b..2773e8f7 100644 --- a/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt +++ b/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt @@ -19,7 +19,6 @@ package com.facebook.ktfmt.cli import com.facebook.ktfmt.format.Formatter import com.facebook.ktfmt.format.FormattingOptions import java.io.File -import java.io.PrintStream import java.nio.charset.StandardCharsets.UTF_8 /** ParsedArgs holds the arguments passed to ktfmt on the command-line, after parsing. */ @@ -39,16 +38,16 @@ data class ParsedArgs( ) { companion object { - fun processArgs(err: PrintStream, args: Array): ParsedArgs { + fun processArgs(args: Array): ParseResult { if (args.size == 1 && args[0].startsWith("@")) { - return parseOptions(err, File(args[0].substring(1)).readLines(UTF_8).toTypedArray()) + return parseOptions(File(args[0].substring(1)).readLines(UTF_8).toTypedArray()) } else { - return parseOptions(err, args) + return parseOptions(args) } } /** parseOptions parses command-line arguments passed to ktfmt. */ - fun parseOptions(err: PrintStream, args: Array): ParsedArgs { + fun parseOptions(args: Array): ParseResult { val fileNames = mutableListOf() var formattingOptions = FormattingOptions() var dryRun = false @@ -64,29 +63,33 @@ data class ParsedArgs( arg == "--dry-run" || arg == "-n" -> dryRun = true arg == "--set-exit-if-changed" -> setExitIfChanged = true arg == "--do-not-remove-unused-imports" -> removeUnusedImports = false - arg.startsWith("--stdin-name") -> stdinName = parseKeyValueArg(err, "--stdin-name", arg) - arg.startsWith("--") -> err.println("Unexpected option: $arg") - arg.startsWith("@") -> err.println("Unexpected option: $arg") + arg.startsWith("--stdin-name=") -> + stdinName = parseKeyValueArg("--stdin-name", arg) + ?: return ParseResult.Error("Found option '${arg}', expected '${"--stdin-name"}='") + arg.startsWith("--") -> return ParseResult.Error("Unexpected option: $arg") + arg.startsWith("@") -> return ParseResult.Error("Unexpected option: $arg") else -> fileNames.add(arg) } } - return ParsedArgs( + return ParseResult.Ok(ParsedArgs( fileNames, formattingOptions.copy(removeUnusedImports = removeUnusedImports), dryRun, setExitIfChanged, stdinName, - ) + )) } - private fun parseKeyValueArg(err: PrintStream, key: String, arg: String): String? { + private fun parseKeyValueArg(key: String, arg: String): String? { val parts = arg.split('=', limit = 2) - if (parts[0] != key || parts.size != 2) { - err.println("Found option '${arg}', expected '${key}='") - return null - } - return parts[1] + return parts[1].takeIf { parts[0] == key || parts.size == 2 } } } } + +sealed interface ParseResult { + data class Ok(val parsedValue: ParsedArgs): ParseResult + data class Error(val errorMessage: String): ParseResult +} + diff --git a/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt b/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt index 9f0919bf..93981d1e 100644 --- a/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt @@ -19,15 +19,13 @@ package com.facebook.ktfmt.cli import com.facebook.ktfmt.format.Formatter import com.facebook.ktfmt.format.FormattingOptions import com.google.common.truth.Truth.assertThat -import java.io.ByteArrayOutputStream -import java.io.FileNotFoundException -import java.io.PrintStream -import kotlin.io.path.createTempDirectory -import kotlin.test.assertFailsWith import org.junit.After import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 +import java.io.FileNotFoundException +import kotlin.io.path.createTempDirectory +import kotlin.test.assertFailsWith @Suppress("FunctionNaming") @RunWith(JUnit4::class) @@ -41,143 +39,106 @@ class ParsedArgsTest { } @Test - fun `files to format are returned and unknown flags are reported`() { - val (parsed, out) = parseTestOptions("foo.kt", "--unknown") - - assertThat(parsed.fileNames).containsExactly("foo.kt") - assertThat(out.toString()).isEqualTo("Unexpected option: --unknown\n") + fun `unknown flags return an error`() { + val result = ParsedArgs.parseOptions(arrayOf("--unknown")) + assertThat(result).isInstanceOf(ParseResult.Error::class.java) } @Test - fun `files to format are returned and flags starting with @ are reported`() { - val (parsed, out) = parseTestOptions("foo.kt", "@unknown") - - assertThat(parsed.fileNames).containsExactly("foo.kt") - assertThat(out.toString()).isEqualTo("Unexpected option: @unknown\n") + fun `unknown flags starting with '@' return an error`() { + val result = ParsedArgs.parseOptions(arrayOf("@unknown")) + assertThat(result).isInstanceOf(ParseResult.Error::class.java) } @Test fun `parseOptions uses default values when args are empty`() { - val (parsed, _) = parseTestOptions("foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(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() - assertThat(parsed.stdinName).isNull() + val defaultFormattingOptions = FormattingOptions() + assertThat(formattingOptions).isEqualTo(defaultFormattingOptions) } @Test - fun `parseOptions recognizes --dropbox-style and rejects unknown flags`() { - val (parsed, out) = parseTestOptions("--dropbox-style", "foo.kt", "--unknown") - - assertThat(parsed.fileNames).containsExactly("foo.kt") - assertThat(parsed.formattingOptions.blockIndent).isEqualTo(4) - assertThat(parsed.formattingOptions.continuationIndent).isEqualTo(4) - assertThat(out.toString()).isEqualTo("Unexpected option: --unknown\n") + fun `parseOptions recognizes --dropbox-style`() { + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--dropbox-style", "foo.kt"))) + assertThat(parsed.formattingOptions).isEqualTo(Formatter.DROPBOX_FORMAT) } @Test fun `parseOptions recognizes --google-style`() { - val (parsed, _) = parseTestOptions("--google-style", "foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--google-style", "foo.kt"))) assertThat(parsed.formattingOptions).isEqualTo(Formatter.GOOGLE_FORMAT) } @Test fun `parseOptions recognizes --dry-run`() { - val (parsed, _) = parseTestOptions("--dry-run", "foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--dry-run", "foo.kt"))) assertThat(parsed.dryRun).isTrue() } @Test fun `parseOptions recognizes -n as --dry-run`() { - val (parsed, _) = parseTestOptions("-n", "foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("-n", "foo.kt"))) assertThat(parsed.dryRun).isTrue() } @Test fun `parseOptions recognizes --set-exit-if-changed`() { - val (parsed, _) = parseTestOptions("--set-exit-if-changed", "foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--set-exit-if-changed", "foo.kt"))) assertThat(parsed.setExitIfChanged).isTrue() } @Test fun `parseOptions defaults to removing imports`() { - val (parsed, _) = parseTestOptions("foo.kt") + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("foo.kt"))) assertThat(parsed.formattingOptions.removeUnusedImports).isTrue() } @Test fun `parseOptions recognizes --do-not-remove-unused-imports to removing imports`() { - val (parsed, _) = parseTestOptions("--do-not-remove-unused-imports", "foo.kt") - assertThat(parsed.formattingOptions.removeUnusedImports).isFalse() - } - - @Test - fun `parseOptions handles dropbox style and --do-not-remove-unused-imports`() { - val (parsed, _) = - parseTestOptions("--do-not-remove-unused-imports", "--dropbox-style", "foo.kt") - assertThat(parsed.formattingOptions.removeUnusedImports).isFalse() - assertThat(parsed.formattingOptions.style).isEqualTo(FormattingOptions.Style.DROPBOX) - } - - @Test - fun `parseOptions handles google style and --do-not-remove-unused-imports`() { - val (parsed, _) = parseTestOptions("--do-not-remove-unused-imports", "--google-style", "foo.kt") + val parsed = + assertSucceeds(ParsedArgs.parseOptions(arrayOf("--do-not-remove-unused-imports", "foo.kt"))) assertThat(parsed.formattingOptions.removeUnusedImports).isFalse() - assertThat(parsed.formattingOptions.style).isEqualTo(FormattingOptions.Style.GOOGLE) } @Test - fun `parseOptions --stdin-name`() { - val (parsed, _) = parseTestOptions("--stdin-name=my/foo.kt") + fun `parseOptions recognizes --stdin-name`() { + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name=my/foo.kt"))) assertThat(parsed.stdinName).isEqualTo("my/foo.kt") } @Test - fun `parseOptions --stdin-name with empty value`() { - val (parsed, _) = parseTestOptions("--stdin-name=") + fun `parseOptions accepts --stdin-name with empty value`() { + val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name="))) assertThat(parsed.stdinName).isEqualTo("") } - @Test - fun `parseOptions --stdin-name without value`() { - val (parsed, out) = parseTestOptions("--stdin-name") - assertThat(out).isEqualTo("Found option '--stdin-name', expected '--stdin-name='\n") - assertThat(parsed.stdinName).isNull() - } - - @Test - fun `parseOptions --stdin-name prefix`() { - val (parsed, out) = parseTestOptions("--stdin-namea") - assertThat(out).isEqualTo("Found option '--stdin-namea', expected '--stdin-name='\n") - assertThat(parsed.stdinName).isNull() - } + @Test + fun `parseOptions --stdin-name without value`() { + val parseResult = ParsedArgs.parseOptions(arrayOf("--stdin-name")) + assertThat(parseResult).isInstanceOf(ParseResult.Error::class.java) + } @Test fun `processArgs use the @file option with non existing file`() { - val out = ByteArrayOutputStream() - val e = assertFailsWith { - ParsedArgs.processArgs(PrintStream(out), arrayOf("@non-existing-file")) + ParsedArgs.processArgs(arrayOf("@non-existing-file")) } assertThat(e.message).contains("non-existing-file (No such file or directory)") } @Test fun `processArgs use the @file option with file containing arguments`() { - val out = ByteArrayOutputStream() val file = root.resolve("existing-file") file.writeText("--google-style\n--dry-run\n--set-exit-if-changed\nFile1.kt\nFile2.kt\n") - val parsed = ParsedArgs.processArgs(PrintStream(out), arrayOf("@" + file.absolutePath)) + val result = ParsedArgs.processArgs(arrayOf("@" + file.absolutePath)) + assertThat(result).isInstanceOf(ParseResult.Ok::class.java) + + val parsed = (result as ParseResult.Ok).parsedValue assertThat(parsed.formattingOptions).isEqualTo(Formatter.GOOGLE_FORMAT) assertThat(parsed.dryRun).isTrue() @@ -185,8 +146,8 @@ class ParsedArgsTest { assertThat(parsed.fileNames).containsExactlyElementsIn(listOf("File1.kt", "File2.kt")) } - private fun parseTestOptions(vararg args: String): Pair { - val out = ByteArrayOutputStream() - return Pair(ParsedArgs.parseOptions(PrintStream(out), arrayOf(*args)), out.toString()) + private fun assertSucceeds(parseResult: ParseResult): ParsedArgs { + assertThat(parseResult).isInstanceOf(ParseResult.Ok::class.java) + return (parseResult as ParseResult.Ok).parsedValue } } From e046dd052737232f6eac61af1eb1da53ac609901 Mon Sep 17 00:00:00 2001 From: Joseph Cooper Date: Mon, 20 May 2024 16:50:16 +0100 Subject: [PATCH 2/3] Rename Main param args to inputArgs --- core/src/main/java/com/facebook/ktfmt/cli/Main.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt index 3b826966..9e0f068d 100644 --- a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt +++ b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt @@ -36,7 +36,7 @@ class Main( private val input: InputStream, private val out: PrintStream, private val err: PrintStream, - private val args: Array + private val inputArgs: Array ) { companion object { @JvmStatic @@ -72,7 +72,7 @@ class Main( fun run(): Int { - val processArgs = ParsedArgs.processArgs(args) + val processArgs = ParsedArgs.processArgs(inputArgs) val parsedArgs = when (processArgs) { is ParseResult.Ok -> processArgs.parsedValue is ParseResult.Error-> exitFatal(processArgs.errorMessage, 1) From 01ac34f118decc8ab72f7249a99095530157c730 Mon Sep 17 00:00:00 2001 From: Joseph Cooper Date: Mon, 20 May 2024 17:50:57 +0100 Subject: [PATCH 3/3] Add parameterized argument parsing test Intended to make testing of combination of CLI parameters easier --- .../ktfmt/cli/ParsedArgsParameterizedTest.kt | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsParameterizedTest.kt diff --git a/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsParameterizedTest.kt b/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsParameterizedTest.kt new file mode 100644 index 00000000..10c7be22 --- /dev/null +++ b/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsParameterizedTest.kt @@ -0,0 +1,87 @@ +package com.facebook.ktfmt.cli + +import com.facebook.ktfmt.format.Formatter +import com.facebook.ktfmt.format.FormattingOptions +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.Parameterized +import org.junit.runners.Parameterized.Parameters + +@RunWith(Parameterized::class) +class ParsedArgsParameterizedTest( + // Description is unused in tests + // only included here to fulfil contract of @Parameterized test runner + private val description: String, + private val expectedResult: ParseResult, + private val inputArgs: Array +) { + + @Test + fun `combination of args gives expected result`() { + val testResult = ParsedArgs.parseOptions(inputArgs) + assertThat(testResult).isEqualTo(expectedResult) + } + + companion object { + + + /** + Creates the list of parameters for ParsedArgsParameterizedTest + Each instance of the parameters is an array containing: + - a description string, which junit outputs to the console + - the expected result of parsing, + - an array of the input arguments + */ + @JvmStatic + @Parameters(name = "{index}: {0} - parseOptions({1})={2}") + fun testData(): Iterable> = + arrayListOf( + testCase( + description = "Parses multiple args successfully", + expectedParseResult = + parseResultOk( + fileNames = listOf("File.kt"), + formattingOptions = Formatter.GOOGLE_FORMAT, + dryRun = true, + setExitIfChanged = true), + "--google-style", + "--dry-run", + "--set-exit-if-changed", + "File.kt"), + testCase( + description = "Last style in args wins", + expectedParseResult = + parseResultOk( + fileNames = listOf("File.kt"), + formattingOptions = Formatter.DROPBOX_FORMAT), + "--google-style", + "--dropbox-style", + "File.kt"), + testCase( + description = "Error when parsing multiple args and one is unknown", + expectedParseResult = ParseResult.Error("Unexpected option: @unknown"), + "@unknown", + "--google-style")) + + private fun testCase( + description: String, + expectedParseResult: ParseResult, + vararg inputArgs: String + ) = arrayOf(description, expectedParseResult, inputArgs) + + private fun parseResultOk( + fileNames: List = emptyList(), + formattingOptions: FormattingOptions = FormattingOptions(), + dryRun: Boolean = false, + setExitIfChanged: Boolean = false, + removedUnusedImports: Boolean = true, + stdinName: String? = null + ): ParseResult.Ok { + val returnedFormattingOptions = + formattingOptions.copy(removeUnusedImports = removedUnusedImports) + return ParseResult.Ok( + ParsedArgs(fileNames, returnedFormattingOptions, dryRun, setExitIfChanged, stdinName)) + } + } +}