-
Notifications
You must be signed in to change notification settings - Fork 0
Implement CLI interface using Spectre.Console.Cli #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added Spectre.Console and Spectre.Console.Cli packages - Implemented CompareCommand with source/target assembly nomenclature - Created TypeRegistrar for DI integration - Updated Program.cs to use Spectre.Console.Cli - Added comprehensive test coverage for CLI - Updated Taskfile.yml with test report generation - Updated README.md with CLI usage examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a complete CLI interface using Spectre.Console.Cli, replacing the basic structure with a fully functional command-line system. The implementation introduces the CompareCommand with source/target assembly nomenclature, comprehensive validation, and proper exit code handling.
Key Changes
- Implemented CompareCommand with comprehensive validation and execution logic
- Added Spectre.Console.Cli packages and created TypeRegistrar for dependency injection integration
- Updated Program.cs to use Spectre.Console.Cli for command-line parsing with proper error handling
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/DotNetApiDiff.Tests/Commands/CompareCommandTests.cs | Comprehensive unit tests for command validation, execution, and exit codes |
| src/DotNetApiDiff/Program.cs | Main program updated to use Spectre.Console.Cli with command configuration |
| src/DotNetApiDiff/DotNetApiDiff.csproj | Added Spectre.Console package references |
| src/DotNetApiDiff/Commands/TypeRegistrar.cs | Dependency injection integration for Spectre.Console.Cli |
| src/DotNetApiDiff/Commands/CompareCommand.cs | Main command implementation with settings, validation, and execution |
| Taskfile.yml | Enhanced with additional test and coverage reporting tasks |
| README.md | Updated with new CLI usage examples and command syntax |
Comments suppressed due to low confidence (1)
src/DotNetApiDiff/Commands/CompareCommand.cs:216
- The variable name 'format' shadows the method parameter name used in the switch expression. Consider renaming to 'reportFormat' for clarity.
ReportFormat format = settings.OutputFormat.ToLowerInvariant() switch
|
|
||
| var report = reportGenerator.GenerateReport(comparisonResult, format); | ||
|
|
||
| // Output report |
Copilot
AI
Jul 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment 'Output report' is not descriptive. Consider expanding it to explain that this outputs the formatted report to the console using AnsiConsole.
| // Output report | |
| // Output the formatted report to the console using the AnsiConsole library |
| Assert.Equal(0, result); | ||
| mockAssemblyLoader.Verify(al => al.LoadAssembly(sourceAssemblyPath), Times.Once); | ||
| mockAssemblyLoader.Verify(al => al.LoadAssembly(targetAssemblyPath), Times.Once); | ||
| mockApiExtractor.Verify(ae => ae.ExtractApiMembers(It.IsAny<System.Reflection.Assembly>()), Times.Exactly(2)); |
Copilot
AI
Jul 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number '2' should be replaced with a named constant or comment explaining why ExtractApiMembers is called exactly twice (once for source, once for target).
| mockApiExtractor.Verify(ae => ae.ExtractApiMembers(It.IsAny<System.Reflection.Assembly>()), Times.Exactly(2)); | |
| const int EXPECTED_API_EXTRACTION_CALLS = 2; // Number of assemblies processed: source and target | |
| mockApiExtractor.Verify(ae => ae.ExtractApiMembers(It.IsAny<System.Reflection.Assembly>()), Times.Exactly(EXPECTED_API_EXTRACTION_CALLS)); |
- SA1515: Added blank lines before single-line comments - SA1011: Added spaces after closing square brackets - SA1206: Reordered modifiers to put 'required' before 'public'
- Improved comment for output report to be more descriptive - Added named constant for API extraction calls in tests
jbrinkman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed the code review comments:
- Improved the comment for the output report to be more descriptive
- Added a named constant with a comment to explain the magic number '2' in the test file
Note that the configuration loading logic will be implemented in a subsequent task as mentioned by @jbrinkman.
|
|
||
| var report = reportGenerator.GenerateReport(comparisonResult, format); | ||
|
|
||
| // Output the formatted report to the console using the AnsiConsole library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've improved this comment to be more descriptive about what's happening here.
| Assert.Equal(0, result); | ||
| mockAssemblyLoader.Verify(al => al.LoadAssembly(sourceAssemblyPath), Times.Once); | ||
| mockAssemblyLoader.Verify(al => al.LoadAssembly(targetAssemblyPath), Times.Once); | ||
| const int EXPECTED_API_EXTRACTION_CALLS = 2; // Number of assemblies processed: source and target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a named constant with a comment to explain why we're verifying that ExtractApiMembers is called exactly twice.
This PR implements task 5.1 from the implementation plan: "Implement command-line interface using Spectre.Console.Cli".
Changes
Requirements Addressed
Testing
The PR includes unit tests for:
Documentation
The README.md has been updated with: