Add option to skip invalid projects#427
Conversation
WalkthroughA new boolean option Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Fix all issues with AI agents
In `@src/NuGetLicense/Program.cs`:
- Around line 36-39: The option skipInvalidProjectsOption incorrectly reuses the
aliases "-t" and "--include-transitive" already used by includeTransitiveOption,
causing System.CommandLine to throw on duplicate aliases; change
skipInvalidProjectsOption to unique flags (e.g., "-s" and
"--skip-invalid-projects" or another distinct pair) in its Option<bool>
constructor and update any code that parses or documents that option to use the
new alias names so there are no conflicting symbols between
skipInvalidProjectsOption and includeTransitiveOption.
🧹 Nitpick comments (2)
tests/NuGetLicense.Test/LicenseValidationOrchestratorTest.cs (1)
219-219: Missing test coverage forSkipInvalidProjects = truebehavior.This only sets the flag to
falseand doesn't assert anything specific to the new option. There's no test that verifies the core feature: whenSkipInvalidProjects = trueand a project throwsInvalidProjectFileException, the exception is silently skipped and processing continues with a zero exit code.Consider adding a test where
_msBuild(or the reader) throwsInvalidProjectFileExceptionfor a project, and asserting thatValidateAsyncreturns0whenSkipInvalidProjects = true(and-1whenfalse).src/NuGetLicense/LicenseValidationOrchestrator.cs (1)
157-163: Silently swallowingInvalidProjectFileExceptionwith no diagnostic output.When
skipInvalidProjectsistrue, the invalid project is skipped without any trace — no warning, no log. Users won't know which projects were skipped or why, making debugging harder. Consider writing a warning to the error stream (or at minimum including the project path in a diagnostic message) so users can see what was skipped.♻️ Sketch — emit a warning when skipping
Since
GetPackagesPerProjectisstatic, one approach is to collect skipped project info alongside exceptions:catch (InvalidProjectFileException ipfe) { if (!skipInvalidProjects) { encounteredExceptions.Add(ipfe); } + else + { + // Consider logging: Console.Error.WriteLine($"Warning: Skipping invalid project '{project}': {ipfe.Message}"); + // Or collect skipped projects and surface them to the caller. + } }
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/NuGetLicense/LicenseValidationOrchestrator.cs`:
- Around line 157-170: The catch blocks for InvalidProjectFileException and
ReferencedPackageReaderException currently drop exceptions when
skipInvalidProjects is true, leaving users unaware of skipped projects; modify
the static method that contains these catches (referencing skipInvalidProjects
and encounteredExceptions) to surface skipped exceptions by either adding an out
IReadOnlyCollection<Exception> skippedExceptions or an Action<Exception>
logSkipped callback parameter, and when skipInvalidProjects is true call the
logger/callback (or add the exception to the out collection) instead of silently
discarding; update all callers to pass the new out parameter or callback and
document that callers should write to _errorStream if they want console
warnings.
- Line 164: The current catch in LicenseValidationOrchestrator.cs using "catch
(ReferencedPackageReaderException rpre) when (rpre.Message.Contains(\"not
compatible with\"))" is dead and misses the "Target framework not found" case
from ReferencedPackageReader.cs; replace the string-filter approach by adding a
discriminant to ReferencedPackageReaderException (e.g., a FailureKind or
ErrorCode property or an IsFrameworkRelated boolean) at the throw sites in
ReferencedPackageReader (including the throw at ReferencedPackageReader.cs:63),
set the appropriate value for framework-related failures, and change the catch
to "catch (ReferencedPackageReaderException rpre) when (rpre.FailureKind ==
FailureKind.FrameworkNotFound)" (or similar), or alternatively, if you must keep
string checks, expand them to cover all framework-related messages and use
rpre.Message.Contains(..., StringComparison.OrdinalIgnoreCase); update all throw
sites accordingly so framework errors are consistently classified and skipped
when skipInvalidProjects is set.
|
@sensslen tiny PR to add a new flag to allow ignoring errors when loading not compatible projects. |
|
@theolivenbaum thank you very much for your contribution. I would like to understand more about the issue you are facing. I'm using nuget-license on a multi language project successfully. Though I have to use the .net framework version due to incompatibilities of msbuild for .net core with vcxproj files. This is the main reason the project still builds against .net framework. Have you tried that version? My main concern here is that nuget-license has always attempted to prioritize correctness over all other aspects. With the newly proposed flag, it's fairly easy to miss nuget packages that may use incompatible licenses... By the way: one benefit of using the netframework version is that native nuget packages used in native code projects are validated alongside all the managed packages... |
|
Hi @sensslen , thanks for the quick response! The issue we faced is related to a I tried to use the exclude projects option, but it seems that it is only applied after this step. As a quick fix, I thought a new flag to skip these not-supported projects. |
Thanks for sharing more details. Would you be able to share a reproduction example? Also please try wehther the .net framework version of nuget-license actually works with these project types. I'm facing similar issues when I try to use the .net core version (dotnet tool install will always install the core version). |
|
Indeed the framework version seems to be able to open it. I'll investigate here and let you know in a bit |
2 participants
Summary by CodeRabbit