feat(cli)!: repeat-flag canonical form + friendly comma-KV error by ultimatile · Pull Request #11 · ultimatile/PkgTemplatesCommandLineInterface.jl

ultimatile · 2026-04-26T20:45:42Z

Summary

Closes #5. Aligns plugin-option input handling on both create and config set with POSIX/GNU/clig.dev convention: repeat-flag is canonical for multi-key plugin options, bracket form [a, b, c] is the only canonical list-value syntax, and any top-level comma in a plugin option bundle is rejected with a friendly error pointing at the canonical alternatives.

The convergence on bracket-form-only resolved a four-iteration whack-a-mole around comma-as-KV-separator detection: comma was previously trying to mean both "KV separator" (anti-pattern, must reject) and "unquoted-string list delimiter" (legitimate). Dropping the latter makes a top-level comma unambiguous and lets a single up-front scan reject every whitespace variant uniformly.

Changes

feat(cli) — plugin options registered with :action => :append_arg, :nargs => '?', :constant => "", :default => nothing. Both CreateCommand.parse_plugin_options and ConfigCommand._apply_set_args gain an AbstractVector branch that merges repeated invocations left-to-right with last-wins on duplicate keys (matches docker/kubectl/podman aggregation).
fix(parser) — new PluginOptionFormatError <: JTCError. PluginOptionParser.parse_kv_string raises it on any top-level comma (outside "..." / '...' / [...]) via a single up-front scan that reuses split_string's quote/bracket bookkeeping. Quoted commas (name="Doe, Jane") and bracket arrays (ignore=[.DS_Store, .vscode]) keep working.
refactor(parser) — drop unquoted-comma → Vector{String} promotion in parse_value. Drop the implicit split(v, ',') adapter for ignore in PackageGenerator.instantiate_plugins. List values are bracket-form-only.
test — contract coverage on every layer (parser, create E2E, config_set E2E), including a cross-form equivalence assertion that --git "ssh=true manifest=false" and --git ssh=true --git manifest=false produce identical Plugin: Git output (compared as a Set so Dict iteration order doesn't matter).

Behaviour change (breaking)

New: --git ssh=true --git manifest=false works (previously last-wins-only)
Improved: --tests "aqua=true,project=true" now fails with a friendly CLI error instead of silently producing Tests.aqua = ["true", "project=true"]. Same for the aqua=true, project=true, aqua=true ,project=true, and aqua=true , project=true whitespace variants.
Breaking: --git "ignore=.DS_Store,.vscode" is now rejected. Use bracket form --git "ignore=[.DS_Store, .vscode]" instead.
Breaking: existing TOML configs containing string-typed ignore = ".DS_Store,.vscode" (saved by older jtc versions) will surface a MethodError from PkgTemplates plugin construction. Re-save with bracket form via jtc config set --git "ignore=[.DS_Store, .vscode]".
Unchanged: --git "ssh=true manifest=false" (space-separated bundle), name="Doe, Jane" (quoted comma), ignore=[.DS_Store, .vscode] (bracket array), [a, b] items inside brackets

Test plan

All existing tests pass (832 → 916, +84)
Repeat-flag merges into single plugin section
Repeat-flag last-wins on duplicate key
Bare --git combined with KV repeat enables Git + sets options
Cross-form equivalence: repeat ≡ bundle (Set comparison, order-independent)
All four whitespace variants of comma-KV produce a CommandResult with success=false naming the offending bundle
Quoted commas, bracket arrays, and =-inside-brackets all keep working
config_set persists merged section to TOML on repeat-flag input

Register every plugin option with `:action => :append_arg` in addition to the existing `nargs='?', constant="", default=nothing` so users can write `--git ssh=true --git manifest=false` to set multiple keys per plugin, matching the POSIX/GNU/clig.dev convention for multi-value flags and how docker/kubectl/podman handle `-e KEY=VALUE`. CreateCommand.parse_plugin_options and ConfigCommand._apply_set_args both gain an AbstractVector branch that merges per-element parse_kv_string results left-to-right with last-wins on duplicate keys. The legacy AbstractString branch stays so direct callers and tests can still pass a single bundle. Help text on every plugin option now lists both forms (repeat or shell-quoted bundle) and explicitly mentions that comma-separated KEY=VALUE is rejected.

Comma-separated KEY=VALUE pairs (e.g. --tests "aqua=true,project=true") silently produced a Vector{String} value with the comma-promoted fragments, mangling the second key/value pair. clig.dev flags comma-separated multi-value options as an anti-pattern because values may legitimately contain commas (and the parser's own `name="Doe, Jane"` contract relies on that). Add a new PluginOptionFormatError <: JTCError and detect the shape in parse_kv_string after the key/value split: when an unquoted, non-bracket value contains both `,` and `=`, raise the error with a message that shows the offending key/value and points at the two canonical forms (repeat the flag, or pass a single shell-quoted bundle). Quoted commas and pure list values like `ignore=.DS_Store,.vscode` are preserved.

Cover the issue #5 contract at every layer the unified parser touches. PluginOptionParser tests: comma-separated KEY=VALUE shapes raise PluginOptionFormatError, and the message includes the offending key/value plus a CLI-flag canonical form. Confirm legitimate comma uses (pure list values, quoted commas, bracket arrays) keep working so the new rejection has no false positives. CreateCommand E2E tests: drive real argv through ArgParse + CreateCommand.execute(dry-run) for repeat-flag merge, last-wins on duplicate keys, bare-flag combined with KV repeat, and cross-form equivalence (the same intent expressed as repeat or as a bundle must yield byte-identical Plugin: Git output). The comma-KV anti-pattern must surface as a CommandResult with success=false carrying the offending value. ConfigCommand tests: the same contract on the persistence path — repeated --git invocations land in a single [default.Git] section with both keys, and a comma-KV bundle is rejected before any TOML write happens.

Copilot

Pull request overview

Aligns plugin-option CLI input handling for create and config set with a canonical repeat-flag form, while rejecting comma-separated KEY=VALUE anti-patterns that previously caused silent corruption.

Changes:

Registers plugin flags with :append_arg and updates create/config plumbing to merge repeated invocations left-to-right with last-wins on duplicates.
Introduces PluginOptionFormatError and adds parser-side rejection for comma-separated KEY=VALUE shapes with a user-facing remediation message.
Adds regression/E2E coverage for repeat-flag behavior and the new comma-KV rejection across parser, create, and config set.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/cli.jl`	Switches dynamic plugin flags to repeatable `:append_arg` and updates help text to document canonical forms and rejection.
`src/create_command.jl`	Adds `AbstractVector` handling to merge repeated plugin flags into one options section.
`src/config_command.jl`	Adds repeated-flag merge semantics for `config set` plugin options and preserves bare-flag enable behavior.
`src/plugin_option_parser.jl`	Adds comma-separated KV rejection and constructs a friendly error with suggested canonical alternatives.
`src/errors.jl`	Adds the new `PluginOptionFormatError` error type and `showerror` implementation.
`src/PkgTemplatesCommandLineInterface.jl`	Exports `PluginOptionFormatError` from the top-level module.
`test/test_plugin_option_parser.jl`	Adds parser-level contract tests for rejecting comma-separated KV and allowing legitimate comma usage.
`test/test_create_command.jl`	Adds E2E tests for repeat-flag merging/last-wins and comma-KV rejection in `create`.
`test/test_config_command.jl`	Adds config-set tests for persisting merged repeated plugin flags and rejecting comma-KV.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-26T20:50:32Z

+            @test_throws PluginOptionFormatError PluginOptionParser.parse_kv_string(
+                "aqua=true,project=true",
+            )
+            @test_throws PluginOptionFormatError PluginOptionParser.parse_kv_string(


Test coverage for the comma-separated KV rejection should include the common variant with whitespace after commas (e.g. "aqua=true, project=true"). With the current implementation this form slips through tokenization and still corrupts the first key’s type, so adding an assertion here would prevent regressions once the parser is fixed.

Suggested change

@test_throws PluginOptionFormatError PluginOptionParser.parse_kv_string(

@test_throws PluginOptionFormatError PluginOptionParser.parse_kv_string(

"aqua=true, project=true",

)

@test_throws PluginOptionFormatError PluginOptionParser.parse_kv_string(

Fixed in b161d61 — added aqua=true, project=true and ssh=true, manifest=false (with space) to the rejection testset.

Copilot · 2026-04-26T20:50:33Z

+        msg = """
+            Plugin option value $(repr(value)) for key $(repr(key)) looks like a
+            comma-separated list of KEY=VALUE pairs, which is not supported.
+            Please use one of:
+              $repeat_form
+              $bundle_form"""


The constructed multi-line msg includes leading indentation/newlines from the triple-quoted string literal. This will surface in CLI output (especially via handle_error paths that use e.message) as oddly indented error text. Consider building the message without leading whitespace or applying strip/dedent before throwing.

Suggested change

msg = """

Plugin option value $(repr(value)) for key $(repr(key)) looks like a

comma-separated list of KEY=VALUE pairs, which is not supported.

Please use one of:

$repeat_form

$bundle_form"""

msg = strip("""

Plugin option value $(repr(value)) for key $(repr(key)) looks like a

comma-separated list of KEY=VALUE pairs, which is not supported.

Please use one of:

$repeat_form

$bundle_form""")

Fixed in b161d61 — message is now built with string(...) and explicit \n so there is no leading whitespace from triple-quote indentation.

The previous comma-KV detection only fired when both `,` and `=` appeared in a single unquoted value. The user-typed shape `aqua=true, project=true` (comma + space) was split into two tokens by split_string before the check, so the trailing comma in the first value and the `=` in the second token both passed inspection separately, silently producing `Dict("aqua" => ["true"], "project" => true)`. Add a token-pair check: when the current value ends with `,` and the next token contains `=`, raise `PluginOptionFormatError` with both pieces reconstructed in the canonical-form suggestions. The original inline shape (no whitespace after comma) is still detected. Also thread an optional `plugin_flag` keyword from create / config callers into `parse_kv_string`, so the error message can suggest the real flag (`--tests`, `--git`) instead of a `<plugin>` placeholder. The message now builds without leading whitespace so it renders cleanly through `handle_error` paths. Update the `PluginOptionFormatError` docstring example to a copy-pasteable single-string form.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-26T21:05:56Z

+                any_bare_flag = true
+            else
+                for (opt_key, opt_val) in PluginOptionParser.parse_kv_string(elem; plugin_flag=flag)
                    section[opt_key] = opt_val


These parse_kv_string(...; plugin_flag=flag) calls can throw PluginOptionFormatError (JTCError). ConfigCommand.execute currently catches all exceptions and formats them with sprint(showerror, e), which will surface PluginOptionFormatError: (type prefix) rather than just the friendly message. Consider special-casing JTCError in the execute catch (or delegating to PkgTemplatesCommandLineInterface.handle_error) so users see the intended error text without internal type names.

Fixed in 2e7e970 — ConfigCommand.execute catch now surfaces e.message directly for JTCError, matching the create-side fix.

handle_error in CreateCommand and the catch-all in ConfigCommand.execute both formatted exceptions through sprint(showerror, e), which prefixed the error type and wrapped the result with "Error:" / "Error executing config command:". For JTCError subtypes that carry an already-curated user-facing message, surface e.message directly so the friendly text the parser writes for the user is not buried. For the split-token comma-KV shape (value ends with `,` and the next token contains `=`), include the next token in the offending-value display so the message shows `"true, project=true"` instead of just `"true,"` — users now see exactly what they typed. The cross-form equivalence test compared raw dry-run text, which depends on Dict iteration order and is therefore flaky across Julia sessions. Parse the Plugin: Git block into a Set of "key = value" lines and compare sets so the test asserts equality of options regardless of print order.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ArgParse `:append_arg` returns `Any[]` for every plugin flag the user did not pass, regardless of `:default => nothing`. `parse_plugin_options` already skips that shape, but `merge_config` treated any non-nothing value as a CLI override and let the empty vectors flow into the merged-options dict. The dry-run plan then printed every absent plugin as `Any[]`, drowning the real merged options in noise. Treat `value isa AbstractVector && isempty(value)` like `nothing` in `merge_config` so absent plugin flags no longer surface in the plan.

A third whitespace variant of the comma-separated KV anti-pattern still slipped through: when the user types `aqua=true ,project=true` (space before comma, no space after), `split_string` produced `["aqua=true", ",project=true"]`. The first token is a clean KV; the second's key parses as `",project"`, and the parser silently stored an option under that bogus name. Detect any token whose stripped key contains `,` and raise `PluginOptionFormatError` with the same canonical-form suggestions, so all three known whitespace placements around comma get the same friendly rejection. Add a contract test guarding the new shape.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

`_reject_comma_separated_kv` ran the split pieces through `filter(!isempty, ...)`. For inputs whose first value is empty (e.g. `aqua=,project=true`), that dropped the first piece entirely and the canonical-form suggestion became `aqua=project=true` — a different option from the one the user typed. Keep the first piece even when empty so the suggestion echoes the actual input (`aqua= --tests project=true`). Empty later pieces from malformed `,,` runs are still dropped to avoid noisy `--flag --flag b=2` artefacts in the suggestion.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The 4-iteration whack-a-mole around comma-as-KV-separator detection came from two distinct kinds of comma sharing one syntax: comma as a KV separator (anti-pattern) and comma as an unquoted-string list delimiter (legitimate today). Drop the latter so a top-level comma is unambiguous and can be rejected with a single rule. parse_value: remove the unquoted-comma → Vector{String} promotion. An unquoted "a,b,c" now stays the literal string. Bracket form `[a, b, c]` becomes the only canonical list-value syntax. parse_kv_string: replace the per-shape `_reject_comma_separated_kv` and `_reject_comma_in_key` checks with a single up-front scan that walks the input character by character and raises `PluginOptionFormatError` on any `,` outside of `"..."`/`'...'`/`[...]`. Reuses split_string's quote semantics (apostrophes inside values are literal) so legitimate commas in quoted strings or bracket arrays survive untouched. All four whitespace variants of the anti-pattern (`a=1,b=2`, `a=1, b=2`, `a=1 ,b=2`, `a=1 , b=2`) now reject through the same code path. PackageGenerator.instantiate_plugins: drop the implicit `split(v, ',')` adapter for `ignore`. With list values restricted to bracket form, a string-typed `ignore` reaching the constructor is the user's bug and must surface as a PkgTemplates MethodError instead of silently being reshaped. CLI help text now documents bracket form for list values. The downstream of an existing TOML `ignore = ".DS_Store,.vscode"` (saved by older jtc versions) is a MethodError at PkgTemplates plugin construction. We accept that as the migration path; converting silent adapters into explicit failures is the goal of this change.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T20:50:30Z

+            if k == "version" && v isa String
                processed_options[k] = VersionNumber(v)
            else
+                # `ignore` and other Vector-typed plugin fields must arrive
+                # as Vector{String}: list values are bracket-form-only
+                # (`ignore=[.DS_Store, .vscode]`) per issue #5. A bare
+                # comma-string falls through to PluginType(; kwargs...)
+                # and surfaces a MethodError, which is the right outcome.
                processed_options[k] = v
            end


This change removes the previous implicit support for comma-delimited list values like ignore=.DS_Store,.vscode: instantiate_plugins no longer splits ignore strings, and PluginOptionParser.parse_kv_string now rejects any top-level comma. This conflicts with the PR description’s claim that ignore=.DS_Store,.vscode (pure list value) remains supported. Please either (a) update the PR description/user docs to reflect the breaking behavior change, or (b) adjust parsing to continue accepting comma-only list values (while still rejecting comma-separated KEY=VALUE) and keep/restore the ignore string-to-vector conversion accordingly.

Updated the PR description to reflect the breaking change. The behaviour is intentional (option C in the design discussion): ignore=.DS_Store,.vscode is rejected, only bracket form ignore=[.DS_Store, .vscode] is accepted. Existing TOML configs with string-comma ignore will now surface a MethodError at PkgTemplates plugin construction — chosen as the migration path so silent adapters become explicit failures.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Two related findings from PR #11 review. `_reject_top_level_comma` walked the input tracking quote/bracket state but never validated balance at end-of-scan. A typo like `a=1[,b=2` (missing `]`) left `bracket_depth > 0` when the comma appeared, so the top-level-comma guard was suppressed and the bundle silently parsed as `Dict("a" => "1[,b=2")`. Same problem with an unterminated quote. Validate balance after the scan and raise PluginOptionFormatError with a hint to close the bracket/quote, so typos in bracket-form list values cannot bypass the comma-KV guardrail. `parse_plugin_options` docstring claimed `String[]` for the absent `:append_arg` shape, but ArgParse actually produces `Any[]` (or any empty AbstractVector — the element type is not contractually fixed). Reword to match the runtime: an empty AbstractVector means "not supplied" regardless of element type.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ultimatile added 3 commits April 27, 2026 05:19

ultimatile requested a review from Copilot April 26, 2026 20:45

Copilot started reviewing on behalf of ultimatile April 26, 2026 20:46 View session