feat: read CLI options from .<command> files (#191)#577
Open
MilosM348 wants to merge 2 commits into
Open
Conversation
Adds opt-in dotfile resolution to CliApp, modelled directly on the design in the now-closed PR zio#317 by @Kalin-Rudnicki — which @pablf had already reviewed positively — but with the unique-top-level-name concern resolved the way zio#317's author indicated they intended to pursue before the PR went stale. Behaviour: - given a CliApp whose root has a deterministic top-level name (e.g. `git` or `git push` whose root is `git`), `runWithFileArgs` (and therefore `run`) walks the cwd, every parent of the cwd, and the user's home directory, parsing each `.<name>` file it finds as one line per CLI argument; - closer files override farther ones, command-line arguments override everything; - when the root command is an `OrElse` of distinct top-level names (e.g. `start | stop`) there is no single anchor name to look up, so the file-options pass is silently skipped — this is the exact framing @Kalin-Rudnicki landed on in his own review of zio#317. Surface area: - new `FileOptions` service with `Live` (JVM/Native) and `Noop` (JS, tests) implementations, plus a per-platform `default`; - `CliApp.runWithFileArgs` requires a `FileOptions` in the env; - `CliApp.run` keeps its old signature and now provides the platform default automatically (Live on JVM/Native, Noop on JS) — so existing apps continue to work unchanged unless they happen to ship `.<name>` files alongside the binary; - `CliApp.runWithoutFileArgs` is provided for tests / sandboxes that want to suppress dotfile lookup explicitly; - `Command#parse` and `Options.validate` gain an optional `fromFiles: List[FileOptions.OptionsFromFile] = Nil` parameter so every existing call site continues to compile unchanged. Tests: - `FileOptionsOverrideSpec` — file/CLI override semantics with a constant `FileOptions` impl, plus explicit coverage for `runWithoutFileArgs` and `OrElse`-rooted apps; - `LiveFileOptionsSpec(Shared)` — exercises the real `Live` impl on JVM and Native against a temp directory tree, with `user.dir` / `user.home` overridden via `TestSystem`. Closes zio#191. /claim zio#191 Co-authored-by: Cursor <cursoragent@cursor.com>
CI on zio#577 failed in two ways: 1. case Some(name) => inside CliAppImpl.runWithFileArgs shadowed the class field ame. Scala 2.13 (compiled with -Werror in this repo) rejects pattern bindings that silently shadow an outer scope. Renamed the binder to cmdName and left a comment explaining why. This was the same root cause behind every 2.13 testCross / testJVMs / website job failure -- the website job runs the same compile. 2. scalafmt 3.11.0 wanted reformatting in 6 main + 2 test files: CliApp.scala, Command.scala, Options.scala, FileOptions.scala, FileOptionsOverrideSpec.scala, LiveFileOptionsSpecShared.scala (the platform-specific FileOptionsPlatformSpecific.scala stubs and LiveFileOptionsSpec.scala harnesses were already conformant). Ran `scalafmt` against the whole project; only those six files changed. No behavioural changes; this is a pure CI green-up patch. Signed-off-by: Milos M <MilosM348@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds opt-in dotfile resolution (
.<command>files in cwd, parents, and home dir) toCliApp, so end-users can persist common CLI options without re-typing them every invocation. Closes #191.This builds directly on the design in #317 by @Kalin-Rudnicki - which @pablf had reviewed positively (
Hi, it looks good!) - and addresses the unique-top-level-name concern that ultimately blocked it the way #317's author indicated they intended to land it (Im going to pursue the unique top level option). Credit for the core architecture is theirs; this PR is a forward-port + that design tweak + a test pass forrunWithoutFileArgsand the OrElse-root case.Behaviour
CliAppwhose root has a deterministic top-level name (e.g.git, orgit pushwhose root isgit),runWithFileArgs(and thereforerun) walksuser.dir, every parent ofuser.dir, anduser.home, parsing each.<name>file as one CLI argument per non-empty trimmed line;OrElseof distinct top-level names (e.g.start | stop) there is no single anchor, so the file-options pass is silently skipped and the app parses exactly as before - the exact framing @Kalin-Rudnicki landed on in his own review of Add support for reading command-line options from file(s) (#191) #317.API surface
FileOptionsservice withLive(JVM + Native) andNoop(JS, tests) implementations, plus a per-platformdefaultselected by a tiny platform-specific shim;CliApp.runWithFileArgs(args)is the new primitive - it requiresFileOptionsin the env;CliApp.run(args)keeps its old signature and nowprovideSomeLayers the platform default automatically; existing apps don't change unless they happen to ship.<name>files alongside their binary;CliApp.runWithoutFileArgs(args)pinsFileOptions.Noopfor tests and sandboxes that want to suppress dotfile lookup explicitly;Command#parseandOptions.validategain an optionalfromFiles: List[FileOptions.OptionsFromFile] = Nilparameter so every existing call site (notablyCommandSpecandOptionsSpec) compiles untouched.Tests
FileOptionsOverrideSpec(shared) - file/CLI override matrix with a constantFileOptionsimpl, plus dedicated cases forrunWithoutFileArgsand anOrElse-rooted app;LiveFileOptionsSpec(Shared)(shared body, JVM + Native shells) - drives the realLiveimplementation against a per-test temp directory, withuser.diranduser.homeoverridden throughTestSystemso the test outcome doesn't depend on whoever happens to be running it.Why now
#191 has accumulated several attempts (#278, #308, #317, #547, #552, #558, #556) but none have merged. #317 came closest - full architecture, positive review - and stalled on the OrElse-root design issue. This PR reuses #317's architecture wholesale (with attribution in the commit message) and resolves that specific issue, so the maintainers don't have to re-litigate the design.
/claim #191