Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor CLI argument parsing #467

Closed
wants to merge 3 commits into from
Closed

Refactor CLI argument parsing #467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8defaca
Refactor CLI argument parsing
May 13, 2024
e046dd0
Rename Main param args to inputArgs
May 20, 2024
01ac34f
Add parameterized argument parsing test
May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions core/src/main/java/com/facebook/ktfmt/cli/Main.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Main(
private val input: InputStream,
private val out: PrintStream,
private val err: PrintStream,
args: Array<String>
private val args: Array<String>
grodin marked this conversation as resolved.
Show resolved Hide resolved
) {
companion object {
@JvmStatic
Expand Down Expand Up @@ -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)
}
grodin marked this conversation as resolved.
Show resolved Hide resolved
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 ...")
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -126,17 +131,17 @@ class Main(
* @param file The file to format. If null, the code is read from <stdin>.
* @return true iff input is valid and already formatted.
*/
private fun format(file: File?): Boolean {
val fileName = file?.toString() ?: parsedArgs.stdinName ?: "<stdin>"
private fun format(file: File?, args: ParsedArgs): Boolean {
val fileName = file?.toString() ?: args.stdinName ?: "<stdin>"
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)
}
Expand All @@ -146,7 +151,7 @@ class Main(
return alreadyFormatted
}

if (parsedArgs.dryRun) {
if (args.dryRun) {
if (!alreadyFormatted) {
out.println(fileName)
}
Expand All @@ -173,4 +178,11 @@ class Main(
throw e
}
}

fun exitFatal(message: String, returnCode: Int): Nothing {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is not private?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Glad you caught that actually, it's part of some refactoring I haven't finished

err.println(message)
exitProcess(returnCode)
}

}

35 changes: 19 additions & 16 deletions core/src/main/java/com/facebook/ktfmt/cli/ParsedArgs.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -39,16 +38,16 @@ data class ParsedArgs(
) {
companion object {

fun processArgs(err: PrintStream, args: Array<String>): ParsedArgs {
fun processArgs(args: Array<String>): 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<String>): ParsedArgs {
fun parseOptions(args: Array<out String>): ParseResult {
val fileNames = mutableListOf<String>()
var formattingOptions = FormattingOptions()
var dryRun = false
Expand All @@ -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"}=<value>'")
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}=<value>'")
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
}

117 changes: 39 additions & 78 deletions core/src/test/java/com/facebook/ktfmt/cli/ParsedArgsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -41,152 +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")
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)
}
hick209 marked this conversation as resolved.
Show resolved Hide resolved

@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 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()
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 assertSucceeds(parseResult: ParseResult): ParsedArgs {
assertThat(parseResult).isInstanceOf(ParseResult.Ok::class.java)
return (parseResult as ParseResult.Ok).parsedValue
}
}
Loading