Migrate CLI parsing to System.CommandLine; fix -c/-k unquoted command (#65)#72
Open
Migrate CLI parsing to System.CommandLine; fix -c/-k unquoted command (#65)#72
Conversation
- Replace CommandLineParser 2.9.1 with System.CommandLine 2.0.0-beta4. - Rewrite Program.cs to declare options via Option<T> and project the parsed result onto the existing CosmosShellOptions POCO. - Pre-process argv so -c/-k (and Windows-style /c, /k) consume all remaining tokens as a single command string. This makes `CosmosDBShell -c help mkitem` work without quotes (issue #65). - LocalizableSentenceBuilder no longer derives from CommandLine.Text.SentenceBuilder; it stays as a static accessor for localized option descriptions used by the new option builder. - Drop unused `using CommandLine[.Text]` references in HelpCommand and ParseDocDBConnectionTests; trim SentenceBuilderTests to the parts that still apply.
- Update -c / -k localized help strings to mention that quoting is optional and Windows-style /c, /k are accepted. - README, docs/navigation.md, docs/programming.md: clarify that app- level options must come before -c / -k since everything after -c / -k is captured as the command. Fix examples that previously placed --connect after -c. - Add NormalizeArgumentsTests covering pass-through, -c/-k consume-rest, /c, /C, /k, /K translation, app options before -c, dangling -c, and option-like tokens being absorbed after -c.
There was a problem hiding this comment.
Pull request overview
This PR migrates CosmosDBShell’s host-process CLI argument parsing from CommandLineParser to System.CommandLine, and introduces an argv normalization step so -c/-k can consume the remaining command tail without requiring quoting (fixing #65). It also updates docs/localized help text and adds unit tests for the new normalization behavior.
Changes:
- Replace CommandLineParser-based parsing with System.CommandLine option definitions and POCO projection in
Program.cs. - Add
NormalizeArgumentsto translate/c//kto-c/-kand collapse the remainder of argv after-c/-kinto a single command string. - Update docs and localized help strings, and add new tests for argument normalization.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/programming.md | Documents new -c/-k consume-rest behavior and option ordering requirement. |
| docs/navigation.md | Updates startup option table to reflect consume-rest and /c//k support. |
| README.md | Updates examples to place app options before -c and show unquoted tails. |
| Directory.Packages.props | Replaces CommandLineParser package version with System.CommandLine. |
| CosmosDBShell/lang/en.ftl | Updates localized help strings for -c/-k to explain consume-rest behavior. |
| CosmosDBShell/Program.cs | Implements System.CommandLine parsing, argv normalization, and custom help/version handling. |
| CosmosDBShell/CosmosDBShell.csproj | Swaps package reference from CommandLineParser to System.CommandLine. |
| CosmosDBShell/Azure.Data.Cosmos.Shell.Util/LocalizableSentenceBuilder.cs | Removes CommandLineParser SentenceBuilder inheritance; keeps localized string accessors. |
| CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/HelpCommand.cs | Removes unused CommandLine.Text using. |
| CosmosDBShell.Tests/UtilTest/SentenceBuilderTests.cs | Removes tests that depended on SentenceBuilder instantiation. |
| CosmosDBShell.Tests/UtilTest/ParseDocDBConnectionTests.cs | Removes unused CommandLine-related usings. |
| CosmosDBShell.Tests/UtilTest/NormalizeArgumentsTests.cs | Adds unit tests for NormalizeArguments behavior (consume-rest + /c//k). |
- BuildHelpText now includes a one-line usage synopsis and the --help / --version pseudo-options, fixing ShellProcessTests.HelpOption_ PrintsHelpAndExitsZero (was missing --version in --help output). - LSP detection (--lsp/--stdio) runs after NormalizeArguments, so a --lsp or --stdio token that is part of a -c/-k command tail is absorbed as command text instead of accidentally starting the LSP server. - Add LocalizedCliResources, a System.CommandLine.LocalizationResources subclass that maps the most common parse errors (unrecognized option/argument, missing value, missing required argument, bad conversion) to the existing help-error-* entries in en.ftl, so the localized error strings authored for the previous parser are not lost. Other messages fall back to the default English text.
Replace the hardcoded synopsis line, command-tail note, and --help/ --version descriptions in BuildHelpText with new help-UsageSynopsis, help-CommandTailNote, help-HelpOptionDescription, and help-VersionOptionDescription entries in en.ftl.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
CosmosDBShell/Program.cs:33
--lsp/--stdiodetection currently scans the fully-normalized argv (args.Contains(...)). If the user runs a command tail that is exactly--lsp/--stdio(e.g.,cosmosdbshell -c --lsp),NormalizeArgumentscollapses the tail into a single token that still equals--lsp, so the shell will incorrectly enter LSP mode instead of executing the command. Consider only honoring--lsp/--stdiowhen they appear before any-c/-ktoken (scan args until-c/-kand ignore anything after), so command tails are always treated as command text.
// Handle LSP mode early, before any other code can write to stdout.
// The LSP protocol requires exclusive access to stdin/stdout.
if (args.Contains("--lsp") || args.Contains("--stdio"))
{
var server = await LspServer.CreateLanguageServerAsync();
await server.WaitForExit;
Introduce TakePreCommandArgs and route the manual --help/--version and --lsp/--stdio interception through it. Previously a single trailing token after -c (e.g. `-c --help`) would still match the element-equality check and be intercepted as an app-level flag instead of being forwarded to the shell command. Add four TakePreCommandArgs unit tests.
…n, doc -k - Stdin-piped script error path now stops the MCP host (and stops StartLspServer's hostTask) before returning, matching the explicit- command error path; previously the early return left the background host task running against the soon-to-be-disposed IHost. - LocalizedCliResources.UnrecognizedCommandOrArgument / UnrecognizedArgument now branches on whether the token starts with `-`: option-shaped tokens still get the localized`Option '...' is unknown` message, but bare positional tokens map to a new `help-error-UnknownArgumentError` (`Unrecognized argument '...'`) so root-level stray positionals are not misreported as options. - docs/navigation.md `-k` row now states explicitly that app-level options must come before `-k`, mirroring the `-c` row.
Rename --clearhistory to --clear-history and --cs to --color-system to match the hyphenated convention used by the other startup options. The original short forms are kept as aliases for back-compat.
Auto-size the option column, add value placeholders (<n>, <endpoint>, <direct|gateway>, [<port>], etc.), split OPTIONS and NOTES sections, and clean up the -c/-k and --color-system descriptions.
Remove --connect-mode default-option binding so stray root arguments remain unrecognized arguments. Centralize MCP host shutdown and await the host task on all early returns that stop the host. Use the user-facing option alias in localized missing-value/required-option diagnostics. Add process-level tests for root unknown arguments and missing --connect values.
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
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.
Migrates CLI parsing from
CommandLineParser(maintenance mode) toSystem.CommandLine, and uses the migration to fix #65.Why
CommandLineParserdoes not have a "consume the remainder" option, so todayCosmosDBShell -c help mkitemparses as-c=helpplus a straymkitemtoken, prints the general help, and ignores the user's intent.System.CommandLineis actively maintained by Microsoft and lets us cleanly model that the rest of the command line belongs to-c/-k.What changed
Commit 1 - Migrate CLI parsing from CommandLineParser to System.CommandLine
CommandLineParser 2.9.1withSystem.CommandLine 2.0.0-beta4inDirectory.Packages.propsand the main project.Program.csto declare options viaOption<T>and project the parsed result onto the existingCosmosShellOptionsPOCO.--mcpnow uses nativeArgumentArity.ZeroOrOneinstead of arg patching.NormalizeArgumentsstep before parsing:/c,/C,/k,/Kto-c/-k.-cor-kis encountered, joins all remaining tokens into a single command string. SoCosmosDBShell -c help mkitemandCosmosDBShell /c help mkitemnow work without quoting (CosmosDBShell -c help mkitem doesn't work #65).LocalizableSentenceBuilderno longer derives fromCommandLine.Text.SentenceBuilder; it stays as a static accessor for localized option descriptions consumed by the new option builder.using CommandLine[.Text]references.Commit 2 - Document and test -c/-k consume-rest behavior
-c/-klocalized help strings to mention that quoting is optional and/c//kare accepted.docs/navigation.md,docs/programming.md: clarify that app-level options must come before-c/-k(since everything after them is captured as the command). Fix examples that previously placed--connectafter-c.NormalizeArgumentsTestscovering pass-through,-c/-kconsume-rest,/c,/C,/k,/Ktranslation, app options before-c, dangling-c, and option-like tokens being absorbed after-c.Behavior notes
--connect) must come before-c/-know. This is consistent withcmd /candbash -c.--help/--versionare intercepted up-front so we keep our own localized version heading. Plain text help is rendered byBuildHelpText; if richer formatting is desired we can later route through Spectre.-c "help mkitem",--connect ... -c "...") continue to work.Validation
dotnet buildof bothCosmosDBShellandCosmosDBShell.Testssucceeds.NormalizeArgumentstests pass.-c "echo hello",-c echo hello world,/c help mkitemagainst the built binary.Fixes #65.