From a147d9a8fb281c556966263527abc92909a2732f Mon Sep 17 00:00:00 2001 From: Joseph Cooper Date: Thu, 16 May 2024 20:41:41 +0100 Subject: [PATCH] Refactor CLI argument parsing Closes #465 --- .../main/java/com/facebook/ktfmt/cli/Main.kt | 5 +- .../java/com/facebook/ktfmt/cli/ParsedArgs.kt | 40 +++--- .../com/facebook/ktfmt/cli/ParsedArgsTest.kt | 118 ++++++------------ 3 files changed, 62 insertions(+), 101 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 1a033ed5..9f316a32 100644 --- a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt +++ b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt @@ -72,10 +72,7 @@ class Main( fun run(): Int { - val parsedArgs = when (val argParseResult = ParsedArgs.processArgs(err, args)) { - is ArgParseResult.Ok -> argParseResult.parsedArgs - is ArgParseResult.Error -> exitFatal(argParseResult.errorMessage, 1) - } + val parsedArgs = ParsedArgs.processArgs(args).getOrElse { exitFatal(it.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 ...") 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 6670c18b..22e8664c 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): ArgParseResult { + fun processArgs(args: Array): ParseResult { if (args.size == 1 && args[0].startsWith("@")) { - return ArgParseResult.Ok(parseOptions(err, File(args[0].substring(1)).readLines(UTF_8).toTypedArray())) + return parseOptions(File(args[0].substring(1)).readLines(UTF_8).toTypedArray()) } else { - return ArgParseResult.Ok(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,34 +63,41 @@ 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).getOrElse { error -> return error } + 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): ParseResult { val parts = arg.split('=', limit = 2) if (parts[0] != key || parts.size != 2) { - err.println("Found option '${arg}', expected '${key}='") - return null + return ParseResult.Error("Found option '${arg}', expected '${key}='") + } - return parts[1] + return ParseResult.Ok(parts[1]) } } } -sealed interface ArgParseResult { - data class Ok(val parsedArgs: ParsedArgs): ArgParseResult - data class Error(val errorMessage: String): ArgParseResult +sealed interface ParseResult { + data class Ok(val parsedValue: V): ParseResult + data class Error(val errorMessage: String): ParseResult } + +inline fun ParseResult.getOrElse(onError: (ParseResult.Error) -> V): V = + when (this) { + is ParseResult.Ok -> this.parsedValue + is ParseResult.Error-> onError(this) + } 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 77539663..9d4000e7 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,146 +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") + val parsed = + assertSucceeds(ParsedArgs.parseOptions(arrayOf("--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") - 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 result = ParsedArgs.processArgs(PrintStream(out), arrayOf("@" + file.absolutePath)) - assertThat(result).isInstanceOf(ArgParseResult.Ok::class.java) + val result = ParsedArgs.processArgs(arrayOf("@" + file.absolutePath)) + assertThat(result).isInstanceOf(ParseResult.Ok::class.java) - val parsed = (result as ArgParseResult.Ok).parsedArgs + val parsed = (result as ParseResult.Ok).parsedValue assertThat(parsed.formattingOptions).isEqualTo(Formatter.GOOGLE_FORMAT) assertThat(parsed.dryRun).isTrue() @@ -188,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): V { + assertThat(parseResult).isInstanceOf(ParseResult.Ok::class.java) + return (parseResult as ParseResult.Ok).parsedValue } }