chore: bump System.CommandLine from 2.0.0+beta.5 to 2.0.0#313
chore: bump System.CommandLine from 2.0.0+beta.5 to 2.0.0#313
Conversation
WalkthroughUpdates System.CommandLine package to stable release 2.0.0, refactors CLI command invocation to use a parse-before-invoke approach, modernizes option definition syntax across RenderOptions and RenderCommand, and updates test infrastructure to align with the new parsing and invocation patterns. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Caller
participant Program as Program.Main
participant RenderCommand
participant ParseResult
rect rgb(200, 220, 255)
note right of Program: New Flow: Parse-First Approach
Caller->>Program: Invoke with args
Program->>RenderCommand: Parse(args)
RenderCommand->>ParseResult: Create result
ParseResult-->>Program: Return parsed result
Program->>RenderCommand: Invoke(result)
RenderCommand-->>Program: Return exit code
Program-->>Caller: int exit code
end
rect rgb(220, 200, 255)
note right of Program: Old Flow: Direct Invocation
Caller->>Program: Invoke with args
Program->>RenderCommand: InvokeAsync(args)
RenderCommand-->>Program: int exit code
Program-->>Caller: int exit code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testing/Didot.Cli.Testing/RenderOptions/StdinTests.cs (1)
10-10: Remove unused and suspicious import.The import
using static System.Runtime.InteropServices.JavaScript.JSType;appears unrelated to CLI testing and is not used anywhere in this file. This looks like it may have been accidentally added.Apply this diff to remove the unused import:
-using static System.Runtime.InteropServices.JavaScript.JSType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Didot.Cli/Didot.Cli.csproj(2 hunks)src/Didot.Cli/Program.cs(1 hunks)src/Didot.Cli/RenderCommand.cs(1 hunks)src/Didot.Cli/RenderOptions.cs(3 hunks)testing/Didot.Cli.Testing/ProgramTests.cs(1 hunks)testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs(3 hunks)testing/Didot.Cli.Testing/RenderOptions/OutputTests.cs(1 hunks)testing/Didot.Cli.Testing/RenderOptions/ParserExtensionTests.cs(3 hunks)testing/Didot.Cli.Testing/RenderOptions/ParserParamsTests.cs(3 hunks)testing/Didot.Cli.Testing/RenderOptions/ParserTests.cs(1 hunks)testing/Didot.Cli.Testing/RenderOptions/SourceTests.cs(5 hunks)testing/Didot.Cli.Testing/RenderOptions/StdinTests.cs(1 hunks)testing/Didot.Cli.Testing/RenderOptions/TemplateTests.cs(1 hunks)testing/Didot.Core.Testing/Didot.Core.Testing.csproj(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
testing/Didot.Cli.Testing/RenderOptions/ParserTests.cs (3)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs (2)
Test(13-22)Test(24-41)src/Didot.Cli/RenderOptions.cs (1)
RenderOptions(12-130)
testing/Didot.Cli.Testing/RenderOptions/OutputTests.cs (3)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs (2)
Test(13-22)Test(24-41)src/Didot.Cli/RenderOptions.cs (1)
RenderOptions(12-130)
testing/Didot.Cli.Testing/RenderOptions/StdinTests.cs (2)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)src/Didot.Cli/RenderOptions.cs (1)
RenderOptions(12-130)
testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs (1)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)
testing/Didot.Cli.Testing/RenderOptions/ParserParamsTests.cs (1)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)
src/Didot.Cli/RenderOptions.cs (2)
src/Didot.Cli/RenderCommandHandler.cs (1)
Dictionary(93-114)src/Didot.Core/StringExtensions.cs (1)
NormalizeExtension(11-17)
testing/Didot.Cli.Testing/RenderOptions/ParserExtensionTests.cs (1)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)
testing/Didot.Cli.Testing/RenderOptions/SourceTests.cs (1)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)
src/Didot.Cli/RenderCommand.cs (1)
src/Didot.Cli/RenderCommandHandler.cs (3)
RenderCommandHandler(13-143)RenderCommandHandler(17-18)Execute(20-76)
testing/Didot.Cli.Testing/RenderOptions/TemplateTests.cs (3)
src/Didot.Cli/RenderCommand.cs (2)
RenderCommand(13-83)RenderCommand(15-82)testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs (2)
Test(13-22)Test(24-41)src/Didot.Cli/RenderOptions.cs (1)
RenderOptions(12-130)
🔇 Additional comments (24)
testing/Didot.Core.Testing/Didot.Core.Testing.csproj (1)
3-3: Verify the scope of this bump—PR title references System.CommandLine, but this file updates Microsoft.NET.Test.Sdk.This file shows a bump to
Microsoft.NET.Test.Sdk(18.0.0 → 18.0.1), which is unrelated to the PR's stated objective of upgradingSystem.CommandLineto 2.0.0.While the patch-level bump itself is safe and recommended, this change introduces scope beyond the PR title. Clarify whether this is:
- A collateral update required by the System.CommandLine upgrade (e.g., new test infrastructure patterns)
- An intentional secondary update to address test SDK issues
- Scope creep that should be split into a separate PR
If this bump is necessary for the System.CommandLine upgrade, consider updating the PR description to document this dependency.
testing/Didot.Cli.Testing/ProgramTests.cs (1)
189-189: LGTM! Error message updated to reflect long-form option name.The updated assertion now expects the long-form option name (
--template) in the error message instead of the short-form (-t), which is more descriptive for users and aligns with the System.CommandLine 2.0.0 stable release behavior.testing/Didot.Cli.Testing/RenderOptions/OutputTests.cs (2)
22-25: LGTM! Tests successfully migrated to System.CommandLine 2.0.0 API.The test now uses the direct
RenderCommand.Parse(args)pattern and accesses values viaresult.GetValueandresult.Errors, which aligns with the new System.CommandLine 2.0.0 stable API.
34-37: LGTM! Consistent migration pattern applied.The test correctly uses the new parsing API and verifies the absence of the output option when not provided.
testing/Didot.Cli.Testing/RenderOptions/ParserParamsTests.cs (1)
19-21: LGTM! Parser parameter tests migrated successfully.All test methods consistently use the new
RenderCommand.Parse(args)API pattern with proper result inspection viaresult.GetValueandresult.Errors.Also applies to: 33-36, 53-56
testing/Didot.Cli.Testing/RenderOptions/EngineExtensionTests.cs (1)
19-21: LGTM! Engine extension tests properly migrated.The tests now use the direct parsing pattern and properly verify engine extension configurations using the new API.
Also applies to: 33-36, 52-55
src/Didot.Cli/Program.cs (1)
48-48: LGTM! Execution flow updated to parse-then-invoke pattern.The change from
InvokeAsync(args)toParse(args).InvokeAsync()adopts System.CommandLine 2.0.0's explicit parsing step, which provides better control over the command execution lifecycle and aligns with the API's design patterns.testing/Didot.Cli.Testing/RenderOptions/SourceTests.cs (2)
3-3: LGTM! Added necessary using directive.The
System.CommandLinenamespace is now imported to support the directRenderCommandusage in the tests.
20-22: LGTM! Source option tests successfully migrated.All test methods have been updated to use the
RenderCommand.Parse(args)pattern with proper error and value inspection. The migration maintains test coverage while adopting the new API.Also applies to: 36-39, 59-62, 79-83
testing/Didot.Cli.Testing/RenderOptions/TemplateTests.cs (2)
19-21: LGTM! Template validation test migrated successfully.The test now uses the direct parsing pattern and properly validates the template value.
30-33: LGTM! Error message updated to long-form option name.The test now expects the long-form
--templatein error messages, which is more descriptive and consistent with System.CommandLine 2.0.0 behavior.src/Didot.Cli/Didot.Cli.csproj (1)
29-29: No issues found. The package upgrade is verified as safe.System.CommandLine version 2.0.0 is confirmed as available on NuGet, and no security vulnerabilities exist for this package version. The upgrade from the beta release to 2.0.0 stable is appropriate and safe to proceed with.
testing/Didot.Cli.Testing/RenderOptions/ParserTests.cs (2)
22-26: LGTM: Test migration to System.CommandLine 2.0.0 API.The test correctly uses the new
RenderCommand.Parse(args)API and accesses values throughresult.GetValue(options.Parser)instead of the deprecated approach. The pattern is consistent with other test files in the PR.
34-37: LGTM: Consistent test pattern for optional option.The test correctly validates that an unprovided optional parser option returns
null, using the same API pattern as the required option test above.testing/Didot.Cli.Testing/RenderOptions/StdinTests.cs (2)
23-27: LGTM: StdIn option tests correctly migrated.The tests properly use the new
RenderCommand.Parse(args)API and validate both the presence and absence of the stdin option throughresult.GetValue(options.StdIn).Also applies to: 35-39, 48-52
60-64: LGTM: Error validation tests correctly updated.The error validation tests now correctly access
result.Errorsand validate error messages using the new API. The error messages match those defined inRenderCommand.csvalidators.Also applies to: 85-89, 110-113
src/Didot.Cli/RenderCommand.cs (3)
18-21: LGTM: DefaultValueFactory correctly initialized.The use of
DefaultValueFactory = _ => []for dictionary/collection options aligns with System.CommandLine 2.0.0 patterns, ensuring empty collections are returned when options are not provided.
23-31: LGTM: Options registered with new API.The migration from
AddOption(...)toOptions.Add(...)correctly uses the System.CommandLine 2.0.0 stable API.
73-81: LGTM: SetAction correctly uses new invocation pattern.The migration to
SetActionwithGetRequiredValuefor required options andGetValuewith null-coalescing for optional options correctly implements the System.CommandLine 2.0.0 invocation pattern.testing/Didot.Cli.Testing/RenderOptions/ParserExtensionTests.cs (1)
19-22: LGTM: Parser extension tests correctly migrated.All test cases (empty, single, and multiple parser extensions) correctly use the new
RenderCommand.Parse(args)API and validate results throughresult.GetValue(options.ParserExtensions)andresult.Errors.Also applies to: 33-41, 55-65
src/Didot.Cli/RenderOptions.cs (4)
14-19: LGTM: Template option correctly defined.The option definition uses the new System.CommandLine 2.0.0 constructor syntax with explicit name, aliases, description, and required/arity properties.
21-26: LGTM: CustomParser correctly extracts and trims token values.The
EngineandParseroptions useCustomParserto extract the first token value and trim it, returning an empty string if no tokens are present. This correctly handles the optional string parsing in System.CommandLine 2.0.0.Also applies to: 56-63
28-36: LGTM: Dictionary options use CustomParser with key-value parsing.All dictionary-based options (
EngineExtensions,Sources,ParserExtensions,ParserParams) correctly useCustomParserto delegate to theParseKeyValuePairshelper method with appropriate separators and normalization functions.Also applies to: 38-46, 65-73, 75-83
93-126: LGTM: ParseKeyValuePairs correctly uses new error reporting API.The helper method correctly uses
result.AddError(...)(lines 109, 120) instead of the deprecated error mechanism, aligning with System.CommandLine 2.0.0 patterns.
| try | ||
| { | ||
| sourcesProvided = result.GetValueForOption(options.Sources) is not null | ||
| && result.GetValueForOption(options.Sources)!.Any(); | ||
| sourcesProvided = (result.GetValue(options.Sources)?.Any() ?? false); | ||
| } | ||
| catch { } | ||
| catch | ||
| { } |
There was a problem hiding this comment.
🧩 Analysis chain
Review empty catch blocks in validators.
The validators use empty catch blocks around GetValue(options.Sources). While this appears defensive, silently swallowing exceptions could hide unexpected issues during parsing.
Consider whether GetValue can actually throw for the Sources option in System.CommandLine 2.0.0, or if there's a safer pattern (e.g., checking if the option was provided before calling GetValue). If exceptions are expected here, consider logging them or at least adding a comment explaining why they're caught and ignored.
Also applies to: 62-67
🏁 Script executed:
#!/bin/bash
# Check if there are other patterns in the codebase for handling optional GetValue calls
rg -n -A2 -B2 'GetValue.*Sources' --type csLength of output: 2043
I'll verify whether GetValue actually throws exceptions in System.CommandLine 2.0.0 and check for other patterns in the codebase.
Let me search for more context about how the Sources option is defined in the RenderCommand:
Now let me examine the RenderCommand code to understand the context better:
Based on my verification, the review comment is accurate. The empty catch blocks around GetValue() in RenderCommand.cs are problematic for several reasons:
-
GetValue can throw exceptions for parsing errors, when no symbol is defined, or when the parsed result cannot be cast to the expected type.
-
Empty catch blocks defeat the purpose of exceptions—when an exception occurs, nothing happens and the program fails for an unknown reason, leaving the application in an unknown state.
-
All official System.CommandLine documentation examples show
GetValue()called without try-catch blocks, suggesting this defensive pattern is unnecessary and non-idiomatic. -
Empty catch blocks hide errors and make debugging difficult, and exceptions should always be logged or handled appropriately.
Remove empty catch blocks and handle exceptions appropriately.
The try-catch blocks at lines 37-42 and 62-67 silently swallow exceptions without any logging or handling. Either:
- Verify that
GetValue()won't throw given the context (e.g., by checkingresult.Errorsbeforehand) and remove the try-catch, or - If exceptions are genuinely expected, catch them with specific exception types, log the issue, and add a comment explaining why this is needed.
Using bare catch { } masks errors and violates C# best practices.
🤖 Prompt for AI Agents
In src/Didot.Cli/RenderCommand.cs around lines 37-42 (and similarly 62-67),
remove the empty catch blocks that silently swallow exceptions from
result.GetValue(); instead either (a) eliminate the try/catch and rely on
pre-checking result.Errors and the presence of the option/symbol before calling
GetValue(), or (b) if an exception can legitimately occur, catch specific
exception types only, log the error with context and add a comment explaining
why the exception is expected; do not use bare catch { } blocks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 87.74% 87.93% +0.18%
==========================================
Files 47 47
Lines 1069 1069
Branches 186 187 +1
==========================================
+ Hits 938 940 +2
Misses 72 72
+ Partials 59 57 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
Bug Fixes
Chores