Refactor CLI argument parsing

Closes facebook#465

Author: Joseph Cooper

Diff for: 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=<name>] [--do-not-remove-unused-imports] File1.kt File2.kt ...")

Diff for: 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<String>): ArgParseResult {
+    fun processArgs(args: Array<String>): ParseResult<ParsedArgs> {
       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<String>): ParsedArgs {
+    fun parseOptions(args: Array<out String>): ParseResult<ParsedArgs> {
       val fileNames = mutableListOf<String>()
       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<String> {
       val parts = arg.split('=', limit = 2)
       if (parts[0] != key || parts.size != 2) {
-        err.println("Found option '${arg}', expected '${key}=<value>'")
-        return null
+        return ParseResult.Error("Found option '${arg}', expected '${key}=<value>'")
+
       }
-      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<out V: Any> {
+  data class Ok<V: Any>(val parsedValue: V): ParseResult<V>
+  data class Error(val errorMessage: String): ParseResult<Nothing>
 }
+
+inline fun <V: Any> ParseResult<V>.getOrElse(onError: (ParseResult.Error) -> V): V =
+  when (this) {
+    is ParseResult.Ok<V> -> this.parsedValue
+    is ParseResult.Error-> onError(this)
+  }

Diff for: 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,155 +39,115 @@ 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=<value>'\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=<value>'\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<FileNotFoundException> {
-          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()
     assertThat(parsed.setExitIfChanged).isTrue()
     assertThat(parsed.fileNames).containsExactlyElementsIn(listOf("File1.kt", "File2.kt"))
   }
 
-  private fun parseTestOptions(vararg args: String): Pair<ParsedArgs, String> {
-    val out = ByteArrayOutputStream()
-    return Pair(ParsedArgs.parseOptions(PrintStream(out), arrayOf(*args)), out.toString())
+  private fun <V : Any> assertSucceeds(parseResult: ParseResult<V>): V {
+    assertThat(parseResult).isInstanceOf(ParseResult.Ok::class.java)
+    return (parseResult as ParseResult.Ok<V>).parsedValue
   }
 }