Further refactoring on CLI (#476)upstream-main

Summary:
This doesn't add any features or fix any bugs, just a refactor

Pull Request resolved: https://github.com/facebook/ktfmt/pull/476

Reviewed By: strulovich

Differential Revision: D58377151

Pulled By: hick209

fbshipit-source-id: 3f7d2d0e707f35df8ebf502a73ff104ffc97ba65

4 files changed, 54 insertions, 73 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 fba648b..2b8dde1 100644
--- a/core/src/main/java/com/facebook/ktfmt/cli/Main.kt
+++ b/core/src/main/java/com/facebook/ktfmt/cli/Main.kt
@@ -32,6 +32,16 @@ import java.nio.charset.StandardCharsets.UTF_8
 import java.util.concurrent.atomic.AtomicInteger
 import kotlin.system.exitProcess
 
+private const val EXIT_CODE_FAILURE = 1
+private const val EXIT_CODE_SUCCESS = 0
+
+private val USAGE =
+    """
+        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 ...
+        Or: ktfmt @file
+        """
+        .trimIndent()
+
 class Main(
     private val input: InputStream,
     private val out: PrintStream,
@@ -56,10 +66,6 @@ class Main(
       }
       val result = mutableListOf<File>()
       for (arg in args) {
-        if (arg == "-") {
-          error(
-              "Error: '-', which causes ktfmt to read from stdin, should not be mixed with file name")
-        }
         result.addAll(
             File(arg).walkTopDown().filter {
               it.isFile && (it.extension == "kt" || it.extension == "kts")
@@ -74,48 +80,44 @@ class Main(
     val parsedArgs =
         when (processArgs) {
           is ParseResult.Ok -> processArgs.parsedValue
-          is ParseResult.Error -> exitFatal(processArgs.errorMessage, 1)
+          is ParseResult.Error -> {
+            err.println(processArgs.errorMessage)
+            return EXIT_CODE_FAILURE
+          }
         }
     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 ...")
-      err.println("Or: ktfmt @file")
-      return 1
+      err.println(USAGE)
+      return EXIT_CODE_FAILURE
     }
 
     if (parsedArgs.fileNames.size == 1 && parsedArgs.fileNames[0] == "-") {
+      // Format code read from stdin
       return try {
         val alreadyFormatted = format(null, parsedArgs)
-        if (!alreadyFormatted && parsedArgs.setExitIfChanged) 1 else 0
+        if (!alreadyFormatted && parsedArgs.setExitIfChanged) EXIT_CODE_FAILURE
+        else EXIT_CODE_SUCCESS
       } catch (e: Exception) {
-        1
+        e.printStackTrace(err)
+        EXIT_CODE_FAILURE
       }
-    } else if (parsedArgs.stdinName != null) {
-      err.println("Error: --stdin-name can only be used with stdin")
-      return 1
     }
 
-    val files: List<File>
-    try {
-      files = expandArgsToFileNames(parsedArgs.fileNames)
-    } catch (e: java.lang.IllegalStateException) {
-      err.println(e.message)
-      return 1
-    }
+    val files: List<File> = expandArgsToFileNames(parsedArgs.fileNames)
 
     if (files.isEmpty()) {
       err.println("Error: no .kt files found")
-      return 1
+      return EXIT_CODE_FAILURE
     }
 
-    val retval = AtomicInteger(0)
+    val retval = AtomicInteger(EXIT_CODE_SUCCESS)
     files.parallelStream().forEach {
       try {
         if (!format(it, parsedArgs) && parsedArgs.setExitIfChanged) {
-          retval.set(1)
+          retval.set(EXIT_CODE_FAILURE)
         }
       } catch (e: Exception) {
-        retval.set(1)
+        e.printStackTrace(err)
+        retval.set(EXIT_CODE_FAILURE)
       }
     }
     return retval.get()
@@ -177,15 +179,4 @@ class Main(
       throw e
     }
   }
-
-  /**
-   * Finishes the process with result `returnCode`.
-   *
-   * **WARNING**: If you call this method, this is the last that will happen and no code after it
-   * will be executed.
-   */
-  private 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 6179f5d..b443afe 100644
--- a/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt
+++ b/core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt
@@ -76,6 +76,18 @@ data class ParsedArgs(
         }
       }
 
+      if (fileNames.contains("-")) {
+        // We're reading from stdin
+        if (fileNames.size > 1) {
+          val filesExceptStdin = fileNames - "-"
+          return ParseResult.Error(
+              "Cannot read from stdin and files in same run. Found stdin specifier '-'" +
+                  " and files ${filesExceptStdin.joinToString(", ")} ")
+        }
+      } else if (stdinName != null) {
+        return ParseResult.Error("--stdin-name can only be specified when reading from stdin")
+      }
+
       return ParseResult.Ok(
           ParsedArgs(
               fileNames,
diff --git a/core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt b/core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt
index 925ad27..33d78cd 100644
--- a/core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt
+++ b/core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt
@@ -18,9 +18,7 @@ package com.facebook.ktfmt.cli
 
 import com.google.common.truth.Truth.assertThat
 import java.io.ByteArrayOutputStream
-import java.io.File
 import java.io.PrintStream
-import java.lang.IllegalStateException
 import java.nio.charset.Charset
 import java.nio.charset.StandardCharsets
 import java.nio.charset.StandardCharsets.UTF_8
@@ -28,7 +26,6 @@ import java.nio.file.Files
 import java.util.concurrent.ForkJoinPool
 import kotlin.io.path.createTempDirectory
 import org.junit.After
-import org.junit.Assert.fail
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -105,16 +102,6 @@ class MainTest {
   }
 
   @Test
-  fun `expandArgsToFileNames - a dash is an error`() {
-    try {
-      Main.expandArgsToFileNames(listOf(root.resolve("foo.bar").toString(), File("-").toString()))
-      fail("expected exception, but nothing was thrown")
-    } catch (e: IllegalStateException) {
-      assertThat(e.message).contains("Error")
-    }
-  }
-
-  @Test
   fun `Using '-' as the filename formats an InputStream`() {
     val code = "fun    f1 (  ) :    Int =    0"
     Main(code.byteInputStream(), PrintStream(out), PrintStream(err), arrayOf("-")).run()
@@ -471,27 +458,6 @@ class MainTest {
   }
 
   @Test
-  fun `--stdin-name can only be used with stdin`() {
-    val code = """fun f () =    println( "hello, world" )"""
-    val file = root.resolve("foo.kt")
-    file.writeText(code, UTF_8)
-
-    val exitCode =
-        Main(
-                emptyInput,
-                PrintStream(out),
-                PrintStream(err),
-                arrayOf("--stdin-name=bar.kt", file.toString()))
-            .run()
-
-    assertThat(file.readText()).isEqualTo(code)
-    assertThat(out.toString(UTF_8)).isEmpty()
-    assertThat(err.toString(testCharset))
-        .isEqualTo("Error: --stdin-name can only be used with stdin\n")
-    assertThat(exitCode).isEqualTo(1)
-  }
-
-  @Test
   fun `Always use UTF8 encoding (stdin, stdout)`() {
     val code = """fun f () =    println( "hello, world" )"""
     val expected = """fun f() = println("hello, world")""" + "\n"
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 72f9bec..ae5461b 100644
--- a/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt
+++ b/core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt
@@ -105,23 +105,35 @@ class ParsedArgsTest {
 
   @Test
   fun `parseOptions recognizes --stdin-name`() {
-    val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name=my/foo.kt")))
+    val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name=my/foo.kt", "-")))
     assertThat(parsed.stdinName).isEqualTo("my/foo.kt")
   }
 
   @Test
   fun `parseOptions accepts --stdin-name with empty value`() {
-    val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name=")))
+    val parsed = assertSucceeds(ParsedArgs.parseOptions(arrayOf("--stdin-name=", "-")))
     assertThat(parsed.stdinName).isEqualTo("")
   }
 
   @Test
-  fun `parseOptions --stdin-name without value`() {
+  fun `parseOptions rejects --stdin-name without value`() {
     val parseResult = ParsedArgs.parseOptions(arrayOf("--stdin-name"))
     assertThat(parseResult).isInstanceOf(ParseResult.Error::class.java)
   }
 
   @Test
+  fun `parseOptions rejects '-' and files at the same time`() {
+    val parseResult = ParsedArgs.parseOptions(arrayOf("-", "File.kt"))
+    assertThat(parseResult).isInstanceOf(ParseResult.Error::class.java)
+  }
+
+  @Test
+  fun `parseOptions rejects --stdin-name when not reading from stdin`() {
+    val parseResult = ParsedArgs.parseOptions(arrayOf("--stdin-name=foo", "file1.kt"))
+    assertThat(parseResult).isInstanceOf(ParseResult.Error::class.java)
+  }
+
+  @Test
   fun `processArgs use the @file option with non existing file`() {
     val e =
         assertFailsWith<FileNotFoundException> {