Make command line handling testable via dependency injection#412
Conversation
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
WalkthroughThe PR adds a CommandLineOptions class and a LicenseValidationHandler that centralizes the license validation pipeline, moving logic out of Program.cs and delegating processing to the handler. It introduces comprehensive tests for LicenseValidationHandler, expands test project dependencies (NSubstitute and TestableIO.System.IO.Abstractions.TestingHelpers), and updates GitHub workflows to pin the vstest-action to specific commit hashes. Several new public properties and a new public handler class/method were added; Program.cs was simplified to use the handler. Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/action.yml:
- Line 115: The workflow's action reference "uses:
josepho0918/vstest-action@78cb5377bbb68282f8130758b0d810b1bb76b7b0" points to a
non‑verifiable repository; replace it with the public Microsoft action (e.g.,
"uses: microsoft/vstest-action@v2" or an explicit, documented tag/sha) in the
action.yml, or if this is an intentional private/internal action, update the
uses entry to a reachable internal repository and add documentation in the repo
explaining why a private action is used and how access is provisioned and
verified (ensure the SHA/tag used is valid and accessible).
In `@src/NuGetLicense/LicenseValidationHandler.cs`:
- Around line 262-275: GetPackageInformations currently disposes
sourceRepositoryProvider before the returned IAsyncEnumerable from
PackageInformationReader.GetPackageInfo is enumerated, causing
ObjectDisposedException; convert GetPackageInformations into an async iterator
(make it async IAsyncEnumerable<ReferencedPackageWithContext>) and remove the
using for sourceRepositoryProvider so the provider stays alive during
enumeration, then iterate over informationReader.GetPackageInfo with await
foreach and yield return each item, disposing sourceRepositoryProvider (and any
other disposable resources like globalPackagesFolderUtility) in a finally block
after enumeration completes; reference GetPackageInformations,
sourceRepositoryProvider/WrappedSourceRepositoryProvider, and
PackageInformationReader.GetPackageInfo to locate where to apply the change.
🧹 Nitpick comments (2)
src/NuGetLicense/CommandLineOptions.cs (1)
8-28: Add XML docs for the public option properties.The class has a summary, but the public properties should also carry
<summary>(and other relevant tags) to meet API documentation requirements.
As per coding guidelines, Public APIs in C# should have XML documentation comments using<summary>,<param>,<returns>,<exception>tags as appropriate.src/NuGetLicense/LicenseValidationHandler.cs (1)
32-60: Add XML docs for the public handler API.Please add
<summary>/<param>/<returns>/<exception>docs for the public class, constructor, andHandleAsync.
As per coding guidelines, Public APIs in C# should have XML documentation comments using<summary>,<param>,<returns>,<exception>tags as appropriate.
|
@copilot In
|
…o async iterator Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Fixed in commit 9f7a17b. Converted |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/NuGetLicense/LicenseValidationHandler.cs`:
- Around line 244-260: In GetLicenseMatcher rename the misspelled local variable
spdxLicemseMatcher to spdxLicenseMatcher and update all references (the variable
declaration and its use in the return expression for new
FileLicenseMatcher.Combine.LicenseMatcher) so the identifier is consistent;
ensure the method returns the correctly named spdxLicenseMatcher alongside the
FileLicenseMatcher.Compare.LicenseMatcher instance.
🧹 Nitpick comments (3)
src/NuGetLicense/LicenseValidationHandler.cs (3)
32-58: Consider adding XML documentation for the public class and constructor.Per coding guidelines, public APIs should have XML documentation comments. The
LicenseValidationHandlerclass and its constructor are public entry points that would benefit from documentation describing their purpose and parameter expectations.📝 Suggested documentation
namespace NuGetLicense { + /// <summary> + /// Handles the license validation workflow for NuGet packages. + /// </summary> public class LicenseValidationHandler { private readonly IFileSystem _fileSystem; private readonly HttpClient _httpClient; private readonly ISolutionPersistanceWrapper _solutionPersistance; private readonly IMsBuildAbstraction _msBuild; private readonly IPackagesConfigReader _packagesConfigReader; private readonly Stream _outputStream; private readonly Stream _errorStream; + /// <summary> + /// Initializes a new instance of the <see cref="LicenseValidationHandler"/> class. + /// </summary> + /// <param name="fileSystem">The file system abstraction for file operations.</param> + /// <param name="httpClient">The HTTP client for downloading license files.</param> + /// <param name="solutionPersistance">The solution persistence wrapper for reading solution files.</param> + /// <param name="msBuild">The MSBuild abstraction for project file operations.</param> + /// <param name="packagesConfigReader">The packages.config reader.</param> + /// <param name="outputStream">The stream for writing output.</param> + /// <param name="errorStream">The stream for writing errors.</param> public LicenseValidationHandler(As per coding guidelines: "Public APIs in C# should have XML documentation comments using
<summary>,<param>,<returns>,<exception>tags as appropriate."
200-213: Exception type may be misleading.
FileNotFoundExceptionis thrown when neitherinputFilenorinputJsonFileis provided, but the actual issue is a missing command-line argument, not a missing file. Consider usingArgumentExceptionorInvalidOperationExceptionfor clearer semantics.♻️ Proposed fix
- throw new FileNotFoundException("Please provide an input file"); + throw new ArgumentException("Please provide an input file using --input or --input-file-json");
141-198: Consider the behavior with malformed or empty JSON files.Several methods use the null-forgiving operator (
!) afterJsonSerializer.Deserialize<T>(). If a JSON file contains onlynullor is malformed, this will result in aNullReferenceExceptionrather than a descriptive error.For a CLI tool, failing fast is acceptable, but you might want to provide clearer error messages for better user experience.
💡 Example improvement for one method
private string[] GetAllowedLicenses(string? allowedLicenses) { if (allowedLicenses == null) { return Array.Empty<string>(); } - return JsonSerializer.Deserialize<string[]>(_fileSystem.File.ReadAllText(allowedLicenses))!; + string[] result = JsonSerializer.Deserialize<string[]>(_fileSystem.File.ReadAllText(allowedLicenses)) + ?? throw new InvalidOperationException($"Failed to deserialize allowed licenses from '{allowedLicenses}'"); + return result; }
sensslen
force-pushed
the
copilot/implement-command-line-evaluation
branch
from
33ffc52 to
6f142a0
Compare
|
CommandLineOptionsclass to hold parsed command line parametersLicenseValidationHandlerclass that accepts options and dependencies via constructorProgram.CreateRootCommand()to use the new handlerOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit