From 7df447924e7e6f660e1667e2769a3e43fc70f324 Mon Sep 17 00:00:00 2001 From: Islon Scherer Date: Thu, 30 Oct 2025 10:08:25 +0100 Subject: [PATCH] Coalesce `pkl format` subcommands into the parent command. (#1263) --- docs/modules/pkl-cli/pages/index.adoc | 41 +++-- .../kotlin/org/pkl/cli/CliFormatterApply.kt | 59 ------- .../kotlin/org/pkl/cli/CliFormatterCheck.kt | 48 ------ .../kotlin/org/pkl/cli/CliFormatterCommand.kt | 146 +++++++++++++++--- .../org/pkl/cli/commands/FormatterCommand.kt | 103 ++++++------ 5 files changed, 192 insertions(+), 205 deletions(-) delete mode 100644 pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterApply.kt delete mode 100644 pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCheck.kt diff --git a/docs/modules/pkl-cli/pages/index.adoc b/docs/modules/pkl-cli/pages/index.adoc index 1a87f3b0..d5ec4932 100644 --- a/docs/modules/pkl-cli/pages/index.adoc +++ b/docs/modules/pkl-cli/pages/index.adoc @@ -733,16 +733,22 @@ pkl shell-completion bash pkl shell-completion zsh ---- -[[command-format-check]] -=== `pkl format check` +[[command-format]] +=== `pkl format` -*Synopsis*: `pkl format check ` +*Synopsis*: `pkl format []` -This command checks for format violations on the given file or directory and print their names to stdout. + -It returns a non-zero status code in case violations are found. +This command formats or checks formatting of Pkl files. + +Exit codes: + +* 0: No violations found. +* 1: An unexpected error happened (ex.: IO error) +* 11: Violations were found. If the path is a directory, recursively looks for files with a `.pkl` extension, or files named `PklProject`. +By default, the input files are formatted, and written to standard out. + ==== Options .--grammar-version @@ -753,29 +759,22 @@ Select the grammar compatibility version for the formatter. New versions are created for each backward incompatible grammar change. ==== -[[command-format-apply]] -=== `pkl format apply` - -*Synopsis*: `pkl format apply [] ` - -This command formats the given files overwriting them. - -If the path is a directory, recursively looks for files with a `.pkl` extension, or files named `PklProject`. - -==== Options - .-s, --silent [%collapsible] ==== -Do not write the name of wrongly formatted files to stdout. +Skip writing to standard out. Mutually exclusive with `--diff-name-only`. ==== -.--grammar-version +.-w, --write [%collapsible] ==== -Default: `2` (latest version) + -Select the grammar compatibility version for the formatter. -New versions are created for each backward incompatible grammar change. +Format files in place, overwriting them. Implies `--diff-name-only`. +==== + +.--diff-name-only +[%collapsible] +==== +Write the path of files with formatting violations to stdout. ==== [[common-options]] diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterApply.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterApply.kt deleted file mode 100644 index c52658b3..00000000 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterApply.kt +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Copyright © 2025 Apple Inc. and the Pkl project authors. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.pkl.cli - -import java.io.IOException -import java.nio.file.Files -import java.nio.file.Path -import kotlin.io.path.writeText -import org.pkl.commons.cli.CliBaseOptions -import org.pkl.commons.cli.CliException -import org.pkl.formatter.GrammarVersion - -class CliFormatterApply( - cliBaseOptions: CliBaseOptions, - paths: List, - grammarVersion: GrammarVersion, - private val silent: Boolean, -) : CliFormatterCommand(cliBaseOptions, paths, grammarVersion) { - - override fun doRun() { - var status = 0 - - for (path in paths()) { - val contents = Files.readString(path) - val (formatted, stat) = format(path, contents) - status = if (status == 0) stat else status - if (stat != 0 || contents == formatted) continue - if (!silent) { - consoleWriter.write(path.toAbsolutePath().toString()) - consoleWriter.appendLine() - consoleWriter.flush() - } - try { - path.writeText(formatted, Charsets.UTF_8) - } catch (e: IOException) { - consoleWriter.write("Could not overwrite `$path`: ${e.message}") - consoleWriter.appendLine() - consoleWriter.flush() - status = 1 - } - } - if (status != 0) { - throw CliException("Formatting violations found.", status) - } - } -} diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCheck.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCheck.kt deleted file mode 100644 index 419c18ba..00000000 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCheck.kt +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright © 2025 Apple Inc. and the Pkl project authors. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.pkl.cli - -import java.nio.file.Files -import java.nio.file.Path -import org.pkl.commons.cli.CliBaseOptions -import org.pkl.commons.cli.CliException -import org.pkl.formatter.GrammarVersion - -class CliFormatterCheck( - cliBaseOptions: CliBaseOptions, - paths: List, - grammarVersion: GrammarVersion, -) : CliFormatterCommand(cliBaseOptions, paths, grammarVersion) { - - override fun doRun() { - var status = 0 - - for (path in paths()) { - val contents = Files.readString(path) - val (formatted, stat) = format(path, contents) - status = if (status == 0) stat else status - if (contents != formatted) { - consoleWriter.write(path.toAbsolutePath().toString()) - consoleWriter.appendLine() - consoleWriter.flush() - status = 1 - } - } - if (status != 0) { - throw CliException("Formatting violations found.", status) - } - } -} diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt index 0d466a14..91c63804 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/CliFormatterCommand.kt @@ -15,48 +15,144 @@ */ package org.pkl.cli +import java.io.File +import java.io.IOException import java.io.Writer +import java.nio.file.Files import java.nio.file.Path -import kotlin.io.path.ExperimentalPathApi +import java.util.stream.Stream import kotlin.io.path.extension import kotlin.io.path.isDirectory import kotlin.io.path.name -import kotlin.io.path.walk +import kotlin.io.path.writeText +import kotlin.math.max import org.pkl.commons.cli.CliBaseOptions import org.pkl.commons.cli.CliCommand +import org.pkl.commons.cli.CliTestException +import org.pkl.core.ModuleSource +import org.pkl.core.runtime.VmUtils +import org.pkl.core.util.IoUtils import org.pkl.formatter.Formatter import org.pkl.formatter.GrammarVersion import org.pkl.parser.GenericParserError -abstract class CliFormatterCommand +class CliFormatterCommand @JvmOverloads constructor( - options: CliBaseOptions, - protected val paths: List, - protected val grammarVersion: GrammarVersion, - protected val consoleWriter: Writer = System.out.writer(), -) : CliCommand(options) { - protected fun format(file: Path, contents: String): Pair { - try { - return Formatter().format(contents, grammarVersion) to 0 - } catch (pe: GenericParserError) { - consoleWriter.write("Could not format `$file`: $pe") - consoleWriter.appendLine() - consoleWriter.flush() - return "" to 1 + private val paths: List, + private val grammarVersion: GrammarVersion, + private val overwrite: Boolean, + private val diffNameOnly: Boolean, + private val silent: Boolean, + private val consoleWriter: Writer = System.out.writer(), + private val errWriter: Writer = System.err.writer(), +) : CliCommand(CliBaseOptions()) { + + private fun format(contents: String): String { + return Formatter().format(contents, grammarVersion) + } + + private fun writeErr(error: String) { + errWriter.write(error) + errWriter.appendLine() + errWriter.flush() + } + + private fun write(message: String) { + if (silent) return + consoleWriter.write(message) + consoleWriter.appendLine() + consoleWriter.flush() + } + + private fun allSources(): Stream { + return paths.distinct().stream().flatMap { path -> + when { + path.toString() == "-" -> Stream.of(ModuleSource.text(IoUtils.readString(System.`in`))) + path.isDirectory() -> + Files.walk(path) + .filter { it.extension == "pkl" || it.name == "PklProject" } + .map(ModuleSource::path) + else -> Stream.of(ModuleSource.path(path)) + } } } - @OptIn(ExperimentalPathApi::class) - protected fun paths(): Set { - val allPaths = mutableSetOf() - for (path in paths) { - if (path.isDirectory()) { - allPaths.addAll(path.walk().filter { it.extension == "pkl" || it.name == "PklProject" }) - } else { - allPaths.add(path) + override fun doRun() { + val status = Status(SUCCESS) + + handleSources(status) + + when (status.status) { + FORMATTING_VIOLATION -> { + // using CliTestException instead of CliException because we want full control on how to + // print errors + throw CliTestException("", status.status) + } + ERROR -> { + if (!silent) { + writeErr("An error occurred during formatting.") + } + throw CliTestException("", status.status) + } + } + } + + private fun handleSources(status: Status) { + for (source in allSources()) { + val path = if (source.uri == VmUtils.REPL_TEXT_URI) Path.of("-") else Path.of(source.uri) + try { + val contents = + if (source.contents != null) { + if (overwrite) { + writeErr("Cannot write to stdin.") + throw CliTestException("", ERROR) + } + source.contents!! + } else { + File(source.uri).readText() + } + + val formatted = format(contents) + if (contents != formatted) { + status.update(FORMATTING_VIOLATION) + if (diffNameOnly || overwrite) { + // if `--diff-name-only` or `-w` is specified, only write file names + write(path.toAbsolutePath().toString()) + } + + if (overwrite) { + path.writeText(formatted, Charsets.UTF_8) + } + } + + if (!diffNameOnly && !overwrite) { + write(formatted) + } + } catch (pe: GenericParserError) { + writeErr("Could not format `$path`: $pe") + status.update(ERROR) + } catch (e: IOException) { + writeErr("IO error while reading `$path`: ${e.message}") + status.update(ERROR) + } + } + } + + companion object { + private const val SUCCESS = 0 + private const val FORMATTING_VIOLATION = 11 + private const val ERROR = 1 + + private class Status(var status: Int) { + fun update(newStatus: Int) { + status = + when { + status == ERROR -> status + newStatus == ERROR -> newStatus + else -> max(status, newStatus) + } } } - return allPaths } } diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt index bb5df7a0..3cb51dfa 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/commands/FormatterCommand.kt @@ -15,9 +15,8 @@ */ package org.pkl.cli.commands +import com.github.ajalt.clikt.core.CliktCommand import com.github.ajalt.clikt.core.Context -import com.github.ajalt.clikt.core.NoOpCliktCommand -import com.github.ajalt.clikt.core.subcommands import com.github.ajalt.clikt.parameters.arguments.argument import com.github.ajalt.clikt.parameters.arguments.multiple import com.github.ajalt.clikt.parameters.options.default @@ -26,75 +25,75 @@ import com.github.ajalt.clikt.parameters.options.option import com.github.ajalt.clikt.parameters.types.enum import com.github.ajalt.clikt.parameters.types.path import java.nio.file.Path -import org.pkl.cli.CliFormatterApply -import org.pkl.cli.CliFormatterCheck -import org.pkl.cli.commands.FormatterCheckCommand.Companion.grammarVersionHelp -import org.pkl.commons.cli.commands.BaseCommand +import org.pkl.cli.CliFormatterCommand import org.pkl.formatter.GrammarVersion -class FormatterCommand : NoOpCliktCommand(name = "format") { - override fun help(context: Context) = "Run commands related to formatting" +class FormatterCommand : CliktCommand(name = "format") { + override fun help(context: Context) = + """ + Format or check formatting of Pkl files. + + Examples: + + ``` + # Overwrite all Pkl files inside `my/folder/`, recursively. + $ pkl format -w my/folder/ + + # Check formatting of all files, printing filenames with formatting violations to stdout. + # Exit with exit code `11` if formatting violations were found. + $ pkl format --diff-name-only my/folder/ + + # Format Pkl code from stdin. + $ echo "foo = 1" | pkl format - + ``` + """ + .trimIndent() override fun helpEpilog(context: Context) = "For more information, visit $helpLink" - init { - subcommands(FormatterCheckCommand(), FormatterApplyCommand()) - } -} - -class FormatterCheckCommand : BaseCommand(name = "check", helpLink = helpLink) { - override val helpString: String = - "Check if the given files are properly formatted, printing the file name to stdout in case they are not. Returns non-zero in case of failure." - val paths: List by - argument(name = "paths", help = "Files or directory to check.") - .path(mustExist = true, canBeDir = true) + argument(name = "paths", help = "Files or directory to check. Use `-` to read from stdin.") + .path(mustExist = false, canBeDir = true) .multiple() val grammarVersion: GrammarVersion by - option(names = arrayOf("--grammar-version"), help = grammarVersionHelp) + option( + names = arrayOf("--grammar-version"), + help = + """ + The grammar compatibility version to use.$NEWLINE + ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = " ") { + val default = if (it == GrammarVersion.latest()) " `(default)`" else "" + "`${it.version}`: ${it.versionSpan}$default" + }} + """ + .trimIndent(), + ) .enum { "${it.version}" } .default(GrammarVersion.latest()) - override fun run() { - CliFormatterCheck(baseOptions.baseOptions(emptyList()), paths, grammarVersion).run() - } + val overwrite: Boolean by + option( + names = arrayOf("-w", "--write"), + help = "Format files in place, overwriting them. Implies `---diff-name-only`.", + ) + .flag(default = false) - companion object { - internal val grammarVersionHelp = - """ - The grammar compatibility version to use.$NEWLINE - ${GrammarVersion.entries.joinToString("$NEWLINE", prefix = " ") { - val default = if (it == GrammarVersion.latest()) " `(default)`" else "" - "`${it.version}`: ${it.versionSpan}$default" - }} - """ - .trimIndent() - } -} - -class FormatterApplyCommand : BaseCommand(name = "apply", helpLink = helpLink) { - override val helpString: String = - "Overwrite all the files in place with the formatted version. Returns non-zero in case of failure." - - val paths: List by - argument(name = "paths", help = "Files or directory to format.") - .path(mustExist = true, canBeDir = true) - .multiple() + val diffNameOnly: Boolean by + option( + names = arrayOf("--diff-name-only"), + help = "Write the path of files with formatting violations to stdout.", + ) + .flag(default = false) val silent: Boolean by option( names = arrayOf("-s", "--silent"), - help = "Do not write the name of the files that failed formatting to stdout.", + help = "Don't write to stdout or stderr. Mutually exclusive with `--diff-name-only`.", ) - .flag() - - val grammarVersion: GrammarVersion by - option(names = arrayOf("--grammar-version"), help = grammarVersionHelp) - .enum { "${it.version}" } - .default(GrammarVersion.latest()) + .flag(default = false) override fun run() { - CliFormatterApply(baseOptions.baseOptions(emptyList()), paths, grammarVersion, silent).run() + CliFormatterCommand(paths, grammarVersion, overwrite, diffNameOnly, silent).run() } }