Skip to content

Replace Spectre.Console.Cli with bv's own CLI layer - #279

Merged
rdeago merged 9 commits into
Tenacom:mainfrom
rdeago:dev/278-replace-spectre-cli
May 27, 2026
Merged

Replace Spectre.Console.Cli with bv's own CLI layer#279
rdeago merged 9 commits into
Tenacom:mainfrom
rdeago:dev/278-replace-spectre-cli

Conversation

@rdeago

@rdeago rdeago commented May 27, 2026

Copy link
Copy Markdown
Member

Closes #278.

Removes the Spectre.Console.Cli dependency and replaces it with bv's own command-line parsing, dispatch, and help rendering. Spectre.Console proper (markup, AnsiConsole, the logger) stays as the console backbone.

What's in here (commit by commit)

  1. Namespace unclutter — split the overloaded Cli namespace into Commands, CommandLine, and Infrastructure.Execution.
  2. Parser primitives + first test projectCliArgSplitter, CliOptionReader, BvOptionAttribute, ParsedCommandLine, plus tests/Buildvana.Tool.Tests (TUnit on Microsoft.Testing.Platform — the repo's first test project).
  3. The flip — own dispatcher (CommandRegistry + IBvCommand), GlobalSettings (merging the old globals + base settings), BvHelpRenderer, the -- forwarding rule, Console.CancelKeyPress wiring; deletes the Spectre adapters and drops the Spectre.Console.Cli package.
  4. Internalize the tool's types now that Spectre no longer forces them public.
  5. Rename CommandsSubcommands (it shadowed LibGit2Sharp.Commands).
  6. Changelog — update the forwarding / -c bullets for the -- separator.

Behavior change

The only intentional user-facing change is -- forwarding: bv build/restore/test/pack forward only what follows -- (e.g. bv build -- -p:Foo=Bar); option-looking tokens before -- now error and point at the separator. bv release rejects --. The rest of the option surface is behavior-preserving.

Test plan

  • dotnet bv pack — clean (Release build + tests + artifacts).
  • 47 unit tests pass (parser, reader, splitter, ReleaseSettings, argument validation, command lookup, help reflection).
  • ReSharper inspectcode --severity=WARNING — 0 results.
  • Manual against the built bv: root/per-command help, --version, and the --/unknown-option/unknown-command error paths (correct messages + exit codes).

rdeago and others added 6 commits May 26, 2026 22:51
…re.Execution

Unclutter the overloaded Cli namespace by extracting cohesive groups:

- Buildvana.Tool.Commands: the command classes plus their settings
  (BaseSettings, ReleaseSettings, GlobalOptions) and BuildSteps.
- Buildvana.Tool.CommandLine: ForwardedArguments (the parsing primitives
  follow in the next commit).
- Buildvana.Tool.Infrastructure.Execution: command discovery and dispatch
  (ImplementsCommandAttribute, CommandRegistration, CommandRegistry).

The Spectre.Console.Cli glue (BvHelpProvider, TypeRegistrar, TypeResolver)
stays in Cli pending its removal. Behavior-preserving: Spectre still drives
parsing and dispatch. GitService aliases LibGit2Sharp.Commands so the new
Commands namespace no longer shadows it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Introduce the hand-rolled command-line layer in Buildvana.Tool.CommandLine,
not yet wired into Program (Spectre still runs the show):

- BvOptionAttribute: template-parsed option help metadata.
- CliOptionReader: long/short names, '=' and space forms, case-insensitive,
  with leftover-token detection for strict commands.
- CliArgSplitter + ParsedCommandLine: split on '--', strip globals/help, then
  classify the residue into subcommand, positionals, and option tokens.

Add the repo's first test project, tests/Buildvana.Tool.Tests, on TUnit and
the Microsoft Testing Platform (29 tests over the reader and splitter). Pin
TUnit and the MTP code-coverage extension centrally, register the project in
the solution, grant InternalsVisibleTo from Buildvana.Tool, and select the
MTP test runner in global.json.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop the Spectre.Console.Cli dependency and own the command-line parsing,
dispatch, and help rendering. Spectre.Console proper (AnsiConsole, markup,
SpectreLogger) stays as the console backbone.

- Program builds its DI container directly and dispatches without Spectre:
  CliArgSplitter parses the line, CommandRegistry resolves the command, and the
  IBvCommand is resolved from the container and executed.
- Commands implement IBvCommand instead of AsyncCommand<TSettings>; the release
  command has its ReleaseSettings injected, parsed from the arg-bag with unknown
  options rejected.
- Merge GlobalOptions and BaseSettings into GlobalSettings, which carries both
  the parsed global values and the [BvOption]/[Description] help metadata.
- Generalize ForwardedArguments into CommandParameters (Options + Forwarded).
- Render help with BvHelpRenderer over Spectre.Console grids, reflecting the
  [BvOption] metadata on GlobalSettings and each command's settings type.
- CommandArgumentValidator enforces the new `--` forwarding rule: forwarding
  commands accept nothing before `--`; non-forwarding commands reject anything
  after it. This replaces the old both-sides forwarding heuristic.
- Wire Console.CancelKeyPress to a CancellationToken handed to ExecuteAsync.
- Delete TypeRegistrar, TypeResolver, BvHelpProvider, BaseSettings, and
  GlobalOptions; drop the Spectre.Console.Cli package reference and version.

Cover the new pieces with tests for ReleaseSettings parsing, argument
validation, command lookup, and settings help reflection (47 tests total).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Spectre.Console.Cli's DI forced command, settings, and service types to be
public. With Spectre gone, make every type in Buildvana.Tool internal
(services, settings, helpers, records, enums); the test project reaches them
through InternalsVisibleTo.

Keep the CA1515 NoWarn: with no public types left, Roslyn still reports a
single locationless CA1515 (a false positive against a synthesized type).
Re-documented; revisit when the analyzers are updated.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Commands namespace shadowed LibGit2Sharp.Commands in GitService (worked
around with a GitCommands alias) and read awkwardly beside the rest of the
command infrastructure. "Subcommands" is unambiguous and more accurate: clean,
build, restore, etc. are subcommands of bv. Rename the namespace and drop the
now-unnecessary GitCommands alias, restoring the plain Commands.Stage call.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The pipeline forwarding and -c/--configuration bullets described the interim
both-sides behavior. Update them for the -- separator rule: pipeline commands
forward only after --, option-looking tokens before -- error, and bv release
rejects -- outright.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions github-actions Bot added area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). area:code [issue/PR] affects project code (excluding tests). area:deps [issue/PR] adds, updates, or removes dependencies. labels May 27, 2026
@rdeago rdeago added enhancement [issue/PR] requests / implements new or improved functionality. breaking [issue/PR] requires / contains breaking changes. refactor [issue/PR] requests / implements a refactor of existing code without affecting functionality. labels May 27, 2026
@rdeago

rdeago commented May 27, 2026

Copy link
Copy Markdown
Member Author

@claude please review this PR.

@claude

claude Bot commented May 27, 2026

Copy link
Copy Markdown

Claude finished @rdeago's task in 5m 39s —— View job


PR Review

  • Gather context and understand the request
  • Read key changed files
  • Analyze correctness, style, and design
  • Post review findings

Overall this is a clean, well-structured migration. The layering is clear (CliArgSplitterCliOptionReaderParsedCommandLineCommandArgumentValidator → dispatch), the test coverage is solid (47 tests across all new units), and ReSharper came back clean. I have three minor observations.


1. Stale "Spectre commands" in BuildSteps.cs XML doc

src/Buildvana.Tool/Subcommands/BuildSteps.cs, line 24:

/// Bodies of the individual pipeline steps (clean / restore / build / test / pack), independent of the
/// Spectre commands that invoke them.

Spectre commands is a leftover from before the migration. It should be bv subcommands (or similar).

Fix this →


2. CommandParameters — class vs record

src/Buildvana.Tool/CommandLine/CommandParameters.cs:

ParsedCommandLine and GlobalSettings are both records. CommandParameters is a class with init-only properties. There's no strong reason for the asymmetry — it serves the same role as a lightweight data carrier passed through DI. A sealed record with two properties would be consistent with its neighbours:

internal sealed record CommandParameters(
    IReadOnlyList<string> Options,
    IReadOnlyList<string> Forwarded);

This is purely a style observation; the class form works correctly.


3. Implicit Spectre markup in Description attributes

src/Buildvana.Tool/Infrastructure/Execution/BvHelpRenderer.cs, line 143 and src/Buildvana.Tool/Subcommands/ReleaseSettings.cs, --bump property:

WriteOptionGrid passes description directly to new Markup(...) without escaping:

grid.AddRow(new Markup(Markup.Escape(names)), new Markup(StripTrailingPeriod(description)));
//                                            ^^^^^^ description is NOT escaped

This is intentional — ReleaseSettings's --bump description uses [bold]none[/] etc. — but the contract is invisible at the point of decoration. A maintainer adding a new option with [value] in the description text (meaning a placeholder, not markup) would get a Spectre markup parse error or silent swallowing of the content.

Worth adding a brief note in BvOptionAttribute's or BvHelpRenderer's doc comment that descriptions are rendered as Spectre markup, not plain text. Alternatively, a comment on Description usage in the existing settings classes would do the same job.


Everything else looks good

  • CliArgSplitter: the two-pass design (separator split → global strip → classify) is simple and easy to follow. The "last wins" semantic for repeated options matches the existing global behavior and is tested.
  • CliOptionReader: the mutating-list approach is clear and the Remaining contract (for catching unknown options) is well-exercised in ReleaseSettings.Parse.
  • CommandArgumentValidator: the forwarding / non-forwarding split is correct. The error message pointing users at -- <arg> is helpful.
  • BvHelpRenderer: the usage of MarkupInterpolated for user-supplied content (command names, option names) correctly auto-escapes them, while the decoration ([grey]...[/]) in the forwarding note is genuine markup. No markup-injection risk.
  • CommandRegistry.Discover(): the pipeline-name guard that fails fast on a typo in PipelineCommandNames is a nice safety net.
  • Test project: using Microsoft.Testing.Platform with TUnit on the first test project in the repo is consistent with the changelog entry about MTP being the only supported runner.
  • Program.Main flow: the --version → logo → null-subcommand → help → validate → execute order is correct. The --color / --no-color XOR logic correctly leaves the console profile untouched when both or neither are set.

@rdeago
rdeago merged commit 8aa4204 into Tenacom:main May 27, 2026
6 checks passed
@rdeago
rdeago deleted the dev/278-replace-spectre-cli branch May 27, 2026 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:code [issue/PR] affects project code (excluding tests). area:deps [issue/PR] adds, updates, or removes dependencies. area:docs [issue/PR] affects documentation (excluding XML documentation that is part of source code). breaking [issue/PR] requires / contains breaking changes. enhancement [issue/PR] requests / implements new or improved functionality. refactor [issue/PR] requests / implements a refactor of existing code without affecting functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Spectre.Console.Cli with bv's own CLI layer

1 participant