fix invalid license paths#353
Conversation
WalkthroughReplaces SPDXLicenseMatcher with a new FileLicenseMatcher implementation (code, tests, and renamed namespaces); adds license-file mapping support and CLI option; updates Program and LicenseValidator to use IFileSystem and IFileLicenseMatcher; normalizes package/license file paths via NuGet.Common.PathUtility.GetPathWithDirectorySeparator in WrappedPackageDownloader and GlobalPackagesFolderUtility; adds an integration project ProjectWithReferenceContainingWindowsSpecificPath and related workflow asset files (licenses, mappings, configs); updates solution/project references and CI workflow matrix/steps to pass the new -file-mapping option. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-07T21:33:27.230ZApplied to files:
🧬 Code graph analysis (1)src/NuGetLicense/LicenseValidator/LicenseValidator.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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 |
sensslen
force-pushed
the
simon/fix-invalid-license-paths
branch
from
ce64726 to
50a948d
Compare
sensslen
force-pushed
the
simon/fix-invalid-license-paths
branch
from
efb1581 to
3916f5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/FileLicenseMatcher/IFileLicenseMatcher.cs (1)
4-9: Clarify the Match contract (empty string vs no match)The interface is fine as-is, but the contract that implementations should return an empty string when there is no match is implicit rather than explicit. Consider adding brief XML docs (and/or a named constant) to spell this out so future implementations behave consistently with the existing callers and tests.
src/FileLicenseMatcher/Compare/LicenseMatcher.cs (1)
16-37: Avoid re-splittinglicenseTexton every iterationBehavior looks correct (whitespace-based token comparison, skipping missing files, empty string when nothing matches), but
licenseTextis split inside the loop each time.You can reduce allocations and CPU a bit by splitting once before the loop:
- public string Match(string licenseText) - { - foreach (KeyValuePair<string, string> kvp in _fileLicenseMap) - { + public string Match(string licenseText) + { + IEnumerable<string> licenseContent = licenseText.Split(Array.Empty<char>(), StringSplitOptions.RemoveEmptyEntries); + foreach (KeyValuePair<string, string> kvp in _fileLicenseMap) + { if (!_fileSystem.File.Exists(kvp.Key)) { continue; } - IEnumerable<string> fileContent = _fileSystem.File.ReadAllText(kvp.Key).Split(Array.Empty<char>(), StringSplitOptions.RemoveEmptyEntries); - IEnumerable<string> licenseContent = licenseText.Split(Array.Empty<char>(), StringSplitOptions.RemoveEmptyEntries); + IEnumerable<string> fileContent = _fileSystem.File.ReadAllText(kvp.Key).Split(Array.Empty<char>(), StringSplitOptions.RemoveEmptyEntries); if (licenseContent.SequenceEqual(fileContent)) { return kvp.Value; } - } + } return string.Empty; }tests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cs (2)
28-36: Avoid capturingStreamReaderinFunc<>closures to prevent lifetime issuesIn all three data sources you do:
using var reader = new StreamReader(executingAssembly.GetManifestResourceStream(name)!); yield return () => new Case(expectedIdentifier, reader.ReadToEnd()); // or reader.ReadToEnd() for stringGiven TUnit’s
MethodDataSourcesemantics forIEnumerable<Func<T>>, theFunc<>can be invoked after the iterator has finished andusing var readerhas disposed the underlying stream. That risksObjectDisposedExceptionor framework‑dependent flakiness, especially in AOT mode. (tunit.dev)Safer pattern is to read the content inside the iterator and capture the string instead of the reader:
-using var reader = new StreamReader(executingAssembly.GetManifestResourceStream(name)!); -yield return () => new Case(expectedIdentifier, reader.ReadToEnd()); +using var reader = new StreamReader(executingAssembly.GetManifestResourceStream(name)!); +var content = reader.ReadToEnd(); +yield return () => new Case(expectedIdentifier, content);and similarly:
-using var reader = new StreamReader(executingAssembly.GetManifestResourceStream(name)!); -yield return () => reader.ReadToEnd(); +using var reader = new StreamReader(executingAssembly.GetManifestResourceStream(name)!); +var content = reader.ReadToEnd(); +yield return () => content;This keeps each
Func<>self‑contained and independent of the disposedStreamReaderwhile still giving each test its ownCaseinstance/string.Also applies to: 45-52, 60-69
73-92: Async assertions with discarded results are fine; consider minor naming cleanupsUsing
_ = await Assert.That(...).Contains(...)/.IsEmpty()is functionally equivalent to a bareawaitand is compatible with TUnit’s async assertion model, so there’s no behavioral concern here. (tunit.dev)Two small nits you might optionally address while touching this area:
- Line 89: method name
Fast_License_Matcher_Should_Mattch_Real_World_Licenseshas a typo in “Mattch”.- The property
FastlicenseMatcher(Line 19) could be renamed toFastLicenseMatcherfor consistent PascalCase and readability.Both are non‑blocking but will improve clarity in test output and code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/action.yml(3 hunks).github/workflows/assets/App/fileToLicenseMapping.json(1 hunks).github/workflows/assets/ProjectWithReferenceContainingFileLicense/fileToLicenseMapping.json(1 hunks).github/workflows/assets/ProjectWithReferenceContainingLicenseExpression/fileToLicenseMapping.json(1 hunks).github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/Licenses/Sonar-Source-Available.txt(1 hunks).github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/fileToLicenseMapping.json(1 hunks).github/workflows/assets/Tests/fileToLicenseMapping.json(1 hunks)NuGetUtility.sln(4 hunks)src/FileLicenseMatcher/Combine/LicenseMatcher.cs(1 hunks)src/FileLicenseMatcher/Compare/LicenseMatcher.cs(1 hunks)src/FileLicenseMatcher/FileLicenseMatcher.csproj(1 hunks)src/FileLicenseMatcher/IFileLicenseMatcher.cs(1 hunks)src/FileLicenseMatcher/SPDX/FastLicenseMatcher.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/ILicenseTemplateOutputHandler.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/InvalidSPDXAnalysisException.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/LicenseParserException.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRule.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRuleException.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/LicenseTextHelper.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/LineColumn.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaCore/SpdxLicenseTemplateHelper.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaLibrary/CompareTemplateOutputHandler.cs(2 hunks)src/FileLicenseMatcher/SPDX/JavaLibrary/DifferenceDescription.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaLibrary/LicenseCompareHelper.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaLibrary/ParseInstruction.cs(1 hunks)src/FileLicenseMatcher/SPDX/JavaLibrary/SpdxCompareException.cs(1 hunks)src/NuGetLicense/LicenseValidator/LicenseValidator.cs(2 hunks)src/NuGetLicense/NuGetLicense.csproj(2 hunks)src/NuGetLicense/Program.cs(11 hunks)src/SPDXLicenseMatcher/LicenseMatcher.cs(0 hunks)tests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.cs(1 hunks)tests/FileLicenseMatcher.Test/Compare/LicenseMatcherTest.cs(1 hunks)tests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csproj(1 hunks)tests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cs(5 hunks)
💤 Files with no reviewable changes (1)
- src/SPDXLicenseMatcher/LicenseMatcher.cs
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/assets/ProjectWithReferenceContainingLicenseExpression/fileToLicenseMapping.json
🚧 Files skipped from review as they are similar to previous changes (15)
- .github/workflows/assets/ProjectWithReferenceContainingFileLicense/fileToLicenseMapping.json
- src/FileLicenseMatcher/SPDX/JavaLibrary/CompareTemplateOutputHandler.cs
- src/FileLicenseMatcher/Combine/LicenseMatcher.cs
- src/NuGetLicense/NuGetLicense.csproj
- src/FileLicenseMatcher/FileLicenseMatcher.csproj
- tests/FileLicenseMatcher.Test/Compare/LicenseMatcherTest.cs
- src/NuGetLicense/Program.cs
- src/FileLicenseMatcher/SPDX/JavaLibrary/ParseInstruction.cs
- src/NuGetLicense/LicenseValidator/LicenseValidator.cs
- src/FileLicenseMatcher/SPDX/JavaCore/LineColumn.cs
- src/FileLicenseMatcher/SPDX/JavaLibrary/SpdxCompareException.cs
- src/FileLicenseMatcher/SPDX/FastLicenseMatcher.cs
- .github/workflows/action.yml
- .github/workflows/assets/App/fileToLicenseMapping.json
- .github/workflows/assets/Tests/fileToLicenseMapping.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
📚 Learning: 2025-11-07T21:33:27.230Z
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
Applied to files:
tests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.csNuGetUtility.slntests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csprojtests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cs
🧬 Code graph analysis (7)
src/FileLicenseMatcher/SPDX/JavaCore/LicenseTextHelper.cs (2)
tests/SPDXLicenseMatcher.Test/LicenseMatcherTest.cs (6)
SPDXLicenseMatcher(4-106)LicenseMatcherTest(6-105)n(29-29)Case(8-8)AllSpdxLicensesFastLicenseMatcher(10-14)Match(13-13)src/SPDXLicenseMatcher/FastLicenseMatcher.cs (1)
SPDXLicenseMatcher(14-181)
tests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.cs (1)
tests/SPDXLicenseMatcher.Test/LicenseMatcherTest.cs (9)
SPDXLicenseMatcher(4-106)LicenseMatcherTest(6-105)n(29-29)NonSPDXLicensesTestSource(39-52)SPDXLicensesTestSource(20-36)Test(85-90)Test(99-104)Match(13-13)Test(92-97)
src/FileLicenseMatcher/SPDX/JavaCore/SpdxLicenseTemplateHelper.cs (2)
tests/SPDXLicenseMatcher.Test/LicenseMatcherTest.cs (8)
SPDXLicenseMatcher(4-106)LicenseMatcherTest(6-105)n(29-29)NonSPDXLicensesTestSource(39-52)SPDXLicensesTestSource(20-36)Case(8-8)Match(13-13)Test(85-90)src/SPDXLicenseMatcher/FastLicenseMatcher.cs (2)
SPDXLicenseMatcher(14-181)FastLicenseMatcher(20-180)
src/FileLicenseMatcher/SPDX/JavaLibrary/LicenseCompareHelper.cs (2)
src/SPDXLicenseMatcher/FastLicenseMatcher.cs (1)
SPDXLicenseMatcher(14-181)src/SPDXLicenseMatcher/JavaLibrary/ParseInstruction.cs (1)
ParseInstruction(11-708)
src/FileLicenseMatcher/Compare/LicenseMatcher.cs (1)
src/SPDXLicenseMatcher/FastLicenseMatcher.cs (2)
SPDXLicenseMatcher(14-181)FastLicenseMatcher(20-180)
tests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csproj (1)
tests/SPDXLicenseMatcher.Test/LicenseMatcherTest.cs (3)
SPDXLicenseMatcher(4-106)NonSPDXLicensesTestSource(39-52)LicenseMatcherTest(6-105)
src/FileLicenseMatcher/SPDX/JavaLibrary/DifferenceDescription.cs (1)
src/SPDXLicenseMatcher/JavaLibrary/ParseInstruction.cs (2)
DifferenceDescription(510-524)setLastOptionalDifference(533-541)
🪛 LanguageTool
.github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/Licenses/Sonar-Source-Available.txt
[style] ~49-~49: Consider a more concise word here.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[style] ~118-~118: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ... and inform Recipients how to obtain it in a reasonable manner on or through a medium customarily used...
(IN_A_X_MANNER)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: check_licenses_net472 (Tests)
- GitHub Check: check_licenses_net472 (App)
- GitHub Check: check_version_command_net472
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingLicenseExpression)
- GitHub Check: test_windows (net472)
- GitHub Check: test_windows (net9.0)
- GitHub Check: test_windows (net10.0)
- GitHub Check: test_windows (net8.0)
🔇 Additional comments (22)
src/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRuleException.cs (1)
11-11: Namespace rename is consistent with FileLicenseMatcher refactorThe updated namespace aligns with the new
FileLicenseMatcher.SPDX.*structure and doesn’t introduce any behavioral changes. Looks good as part of the consolidation away fromSPDXLicenseMatcher.*..github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/fileToLicenseMapping.json (1)
1-3: Well-formed JSON with correct path normalization.The mapping correctly uses forward slashes for cross-platform compatibility, and the file path aligns with the actual license file location. This approach properly demonstrates Windows-specific path handling for the asset.
.github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/Licenses/Sonar-Source-Available.txt (1)
1-184: Official license text appropriately included.The complete SONAR Source-Available License v1.0 is properly added as a test asset. The file content should remain unmodified since it represents the official license terms.
Note: Static analysis flagged style suggestions within this license text, but these are false positives—official license text should not be altered for style preferences to maintain legal accuracy and integrity.
src/FileLicenseMatcher/SPDX/JavaCore/LicenseTextHelper.cs (1)
15-15: Namespace move is fine; just confirm external consumer impactMoving
LicenseTextHelpertoFileLicenseMatcher.SPDX.JavaCorealigns with the new FileLicenseMatcher layout and doesn’t change behavior. Because the class ispublic, this is a breaking namespace change for any external callers referencingSPDXLicenseMatcher.JavaCore.LicenseTextHelper; please confirm all in‑repo and downstream consumers are updated or intentionally dropped.src/FileLicenseMatcher/SPDX/JavaCore/InvalidSPDXAnalysisException.cs (1)
11-11: Namespace rename is straightforward; verify external impact of this public typeChanging the namespace to
FileLicenseMatcher.SPDX.JavaCoreis consistent with the broader refactor and is technically fine, butInvalidSPDXAnalysisExceptionis public, so any external code using the oldSPDXLicenseMatcher.JavaCorenamespace will break. Please verify that:
- All references inside this repo are updated to the new namespace, and
- If this assembly is consumed externally (e.g., via NuGet), you’re comfortable with the breaking change or have a migration strategy (type forwarders, major version bump, or a transitional package).
NuGetUtility.sln (4)
3-4: Verify Visual Studio version bump is intentional.The Visual Studio version has been bumped from 17 to 18 (VS 2022 17.11+). Ensure all team members have compatible IDE versions to avoid solution loading issues.
58-65: LGTM! New projects properly structured.The new projects (FileLicenseMatcher, FileLicenseMatcher.Test, and integration test projects) are correctly added with unique GUIDs and follow the established folder structure.
415-468: LGTM! Build configurations properly set.All new projects have complete configuration mappings across Debug, Release, and TestWindows configurations for all platforms (Any CPU, x64, x86).
494-496: LGTM! Project nesting is correct.The new projects are properly nested within their respective solution folders (integration, src, tests).
Based on learnings, the previous SPDXLicenseMatcher.Test.dll was intentionally excluded from release workflow tests using the pattern
NuGet*.Test.dll. Since FileLicenseMatcher.Test replaces SPDXLicenseMatcher.Test, verify whether FileLicenseMatcher.Test.dll should also be excluded from release workflow tests or if the pattern needs updating.src/FileLicenseMatcher/SPDX/JavaCore/SpdxLicenseTemplateHelper.cs (1)
13-13: No action needed.SpdxLicenseTemplateHelperis part of an internal project structure (FileLicenseMatcher is a project dependency referenced in NuGetLicense, not a published library). The namespace change is a clean internal refactor with no external API compatibility concerns.Likely an incorrect or invalid review comment.
src/FileLicenseMatcher/SPDX/JavaCore/LicenseParserException.cs (1)
11-11: Namespace move looks correctNamespace
FileLicenseMatcher.SPDX.JavaCorematches the new layout; no behavioral impact.src/FileLicenseMatcher/SPDX/JavaCore/ILicenseTemplateOutputHandler.cs (1)
9-9: Namespace rename is consistentInterface is unchanged apart from moving to
FileLicenseMatcher.SPDX.JavaCore; this aligns with the new SPDX organization.src/FileLicenseMatcher/SPDX/JavaLibrary/DifferenceDescription.cs (1)
5-7: Using and namespace updates are alignedSwitch to
FileLicenseMatcher.SPDX.JavaCoreandFileLicenseMatcher.SPDX.JavaLibrarymatches the new SPDX layout;DifferenceDescriptionbehavior remains unchanged.src/FileLicenseMatcher/SPDX/JavaCore/LicenseTemplateRule.cs (1)
13-13: Namespace relocation only
LicenseTemplateRuleis just moved toFileLicenseMatcher.SPDX.JavaCore; parsing and validation logic are untouched.src/FileLicenseMatcher/SPDX/JavaLibrary/LicenseCompareHelper.cs (1)
10-13: SPDX helper now correctly references FileLicenseMatcher namespacesThe added
using FileLicenseMatcher.SPDX.JavaCore;and namespaceFileLicenseMatcher.SPDX.JavaLibrarykeepLicenseCompareHelperwired up to the moved SPDX JavaCore/JavaLibrary types without changing behavior.tests/FileLicenseMatcher.Test/Combine/LicenseMatcherTest.cs (1)
1-60: Combine matcher tests cover key behaviorsThe tests clearly assert the intended semantics for the combinator (empty result when no matchers, first non-empty match wins, and short-circuiting). They also encode the empty-string-as-"no match" convention used by
IFileLicenseMatcher. Looks good.tests/FileLicenseMatcher.Test/FileLicenseMatcher.Test.csproj (4)
18-18: NSubstitute dependency addition is appropriate.The addition of NSubstitute 5.3.0 is a common mocking library for unit tests and aligns with the test infrastructure for the new FileLicenseMatcher implementation.
28-28: ProjectReference path is correct.The FileLicenseMatcher.csproj file exists at
src/FileLicenseMatcher/FileLicenseMatcher.csproj, and the relative path..\..\src\FileLicenseMatcher\FileLicenseMatcher.csprojfrom the test project directory resolves correctly.
32-34: The embedded resource paths and test code PREFIX patterns are correctly aligned.Verification confirms:
- All three resource directories exist under
tests/FileLicenseMatcher.Test/SPDX/(SPDXLicenses, NonSpdxTestLicenses, RealLicenses)- Test code PREFIX constants in
LicenseMatcherTest.csare properly updated with theSPDX.segment:"FileLicenseMatcher.Test.SPDX.SPDXLicenses.","FileLicenseMatcher.Test.SPDX.NonSpdxTestLicenses.", and"FileLicenseMatcher.Test.SPDX.RealLicenses."- 665 resource files are present across the three directories
The reorganization has been properly implemented throughout the codebase.
24-24: Verify.TUnit 31.8.0 is compatible with TUnit 1.3.15.Verify.TUnit requires TUnit.Core >= 0.87.8, and TUnit 1.3.15 ships with TUnit.Core in the 1.x series (e.g., 1.2.11), which satisfies this constraint. The patch update is safe.
tests/FileLicenseMatcher.Test/SPDX/LicenseMatcherTest.cs (2)
4-16: Namespace and matcher wiring look consistent with new FileLicenseMatcher.SPDX API
using FileLicenseMatcher.SPDX;, theFileLicenseMatcher.Test.SPDXnamespace, andAllSpdxLicensesFastLicenseMatcher : IFileLicenseMatcherall line up with the new matcher abstraction and TUnit’sClassDataSourcepattern; no issues from a wiring or DI perspective.One minor nit (pre‑existing):
FastlicenseMatcher(Line 19) violates usual PascalCase (FastLicenseMatcher). Consider renaming when convenient to keep property names aligned with .NET conventions.Also, given the earlier workflow pattern
NuGet*.Test.dllintentionally excluded the old SPDX tests, please double‑check that the new test assembly name still matches your intended CI coverage (i.e., either remains excluded or is now intentionally included). Based on learnings.
25-60: Updated embedded‑resource prefixes must match assembly default namespaceThe new
PREFIXvalues:
- Line 25:
"FileLicenseMatcher.Test.SPDX.SPDXLicenses."- Line 44:
"FileLicenseMatcher.Test.SPDX.NonSpdxTestLicenses."- Line 58:
"FileLicenseMatcher.Test.SPDX.RealLicenses."are correct assuming:
- The test project’s default namespace is
FileLicenseMatcher.Test.SPDX, and- The embedded resources live under
SPDXLicenses,NonSpdxTestLicenses, andRealLicensesfolders withBuild Action = Embedded resource.If either the default namespace or folder structure differs,
GetManifestResourceNames()will return names that don’t start with these prefixes and the data sources will silently yield zero cases. Please run the tests once with--verbosity normal(or inspect the DLL with something likeildasm/dotnet toolof your choice) to confirm the resource names actually start with these prefixes.
sensslen
force-pushed
the
simon/fix-invalid-license-paths
branch
from
74266a4 to
120cb7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/action.yml(5 hunks).github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/fileToLicenseMapping.json(1 hunks).github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/ignorePackages.json(1 hunks)integration/ProjectWithReferenceContainingWindowsSpecificPath/ProjectWithReferenceContainingWindowsSpecificPath.csproj(1 hunks)src/FileLicenseMatcher/Compare/LicenseMatcher.cs(1 hunks)src/NuGetLicense/Program.cs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- integration/ProjectWithReferenceContainingWindowsSpecificPath/ProjectWithReferenceContainingWindowsSpecificPath.csproj
- .github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/fileToLicenseMapping.json
- src/FileLicenseMatcher/Compare/LicenseMatcher.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
📚 Learning: 2025-11-07T21:33:27.230Z
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
Applied to files:
.github/workflows/action.yml
🧬 Code graph analysis (1)
src/NuGetLicense/Program.cs (1)
src/NuGetUtility/Wrapper/HttpClientWrapper/NopFileDownloader.cs (1)
NopFileDownloader(6-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingWindowsSpecificPath)
- GitHub Check: check_version_command (net10.0)
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingLicenseExpression)
- GitHub Check: check_version_command (net8.0)
- GitHub Check: check_licenses_net472 (App)
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingFileLicense)
- GitHub Check: check_licenses_net472 (Tests)
- GitHub Check: test_windows (net9.0)
- GitHub Check: check_version_command_net472
- GitHub Check: test_windows (net472)
- GitHub Check: test_windows (net10.0)
- GitHub Check: test_windows (net8.0)
- GitHub Check: test (macos-latest, net9.0)
- GitHub Check: test_file_license_matching (windows-latest)
- GitHub Check: test (macos-latest, net8.0)
- GitHub Check: test_file_license_matching (macos-latest)
- GitHub Check: test_file_license_matching (ubuntu-latest)
🔇 Additional comments (10)
.github/workflows/assets/ProjectWithReferenceContainingWindowsSpecificPath/ignorePackages.json (1)
1-3: JSON structure is valid.The ignorePackages.json file is syntactically valid and appropriately ignores NETStandard.Library for the Windows-specific path project.
.github/workflows/action.yml (3)
147-147: All required asset files are properly configured for the new project. The matrix expansion to includeProjectWithReferenceContainingWindowsSpecificPathat lines 147 and 184 is fully supported by complete asset directories with all required configuration files.
169-169: Ensure -file-mapping parameter files exist for all projects.The
-file-mappingparameter is added to both check_licenses and check_licenses_net472 jobs, referencingfileToLicenseMapping.jsonfor each project. Verify that this file exists for all five projects in the matrix (App, Tests, ProjectWithReferenceContainingLicenseExpression, ProjectWithReferenceContainingFileLicense, ProjectWithReferenceContainingWindowsSpecificPath).Also applies to: 215-215
72-72: No changes needed. The FileLicenseMatcher.Test project targets only net9.0, so the absence of a framework matrix is correct and intentional. Unlike the other test jobs that run against multiple framework versions (net8.0, net9.0, net10.0), this test project does not require framework matrix testing. The OS matrix (ubuntu-latest, macos-latest, windows-latest) appropriately handles platform-specific compatibility verification.src/NuGetLicense/Program.cs (6)
5-5: LGTM! Good refactoring for testability.The addition of
System.IO.Abstractionsenables testable file system operations, andFileLicenseMatcherprovides the new license matching capabilities.Also applies to: 8-8
119-122: LGTM! Clear and consistent option definition.The new command-line option follows the existing naming conventions and provides a clear description of its purpose.
132-149: LGTM! Systematic refactoring to use IFileSystem abstraction.The file system abstraction is properly initialized and consistently passed to all dependent methods. This improves testability while maintaining clean production code.
178-185: LGTM! Proper use of IFileSystem abstraction.The method correctly delegates file operations to the provided file system abstraction, maintaining the same functionality while improving testability.
223-236: LGTM! Consistent refactoring for directory operations.Directory existence checks and creation now properly use the file system abstraction.
246-318: LGTM! Consistent refactoring across all file-reading methods.All configuration file reading operations now properly use the file system abstraction, maintaining consistency throughout the codebase.
…ion behavior (#354) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/licensefile-mappings-json.md (1)
41-48: Add language identifier to fenced code block.For consistency and to satisfy the linter, specify a language identifier for the fenced code block. Use
textorplaintextfor directory structures.Apply this diff:
-``` +```text project/ ├── config/ │ └── license-mappings.json └── licenses/ ├── MIT.txt └── Apache-2.0.txtBased on static analysis hints. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d9ba2654f29e16c7ec0a4ede7f8566df02ebf64c and c60ab839badfe3111b0d666a23d2b87df5802504. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `README.md` (2 hunks) * `docs/licensefile-mappings-json.md` (1 hunks) * `src/NuGetLicense/Program.cs` (11 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly patternNuGet*.Test.dllintentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.</details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>src/NuGetLicense/Program.cs (1)</summary><blockquote> <details> <summary>src/NuGetUtility/Wrapper/HttpClientWrapper/NopFileDownloader.cs (1)</summary> * `NopFileDownloader` (6-11) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/licensefile-mappings-json.md</summary> 41-41: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)</summary> * GitHub Check: check_licenses (net9.0, App) * GitHub Check: check_licenses (net8.0, Tests) * GitHub Check: check_licenses (net9.0, ProjectWithReferenceContainingWindowsSpecificPath) * GitHub Check: check_licenses (net9.0, Tests) * GitHub Check: check_licenses (net8.0, ProjectWithReferenceContainingLicenseExpression) * GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingWindowsSpecificPath) * GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingFileLicense) * GitHub Check: check_licenses_net472 (App) * GitHub Check: check_code_format * GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingLicenseExpression) * GitHub Check: check_licenses_net472 (Tests) * GitHub Check: check_version_command_net472 * GitHub Check: test_windows (net472) * GitHub Check: test_windows (net9.0) * GitHub Check: test_windows (net10.0) * GitHub Check: test_windows (net8.0) * GitHub Check: test_file_license_matching (ubuntu-latest) * GitHub Check: test_url_to_license_mapping * GitHub Check: test_file_license_matching (macos-latest) * GitHub Check: test_file_license_matching (windows-latest) </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>README.md (2)</summary><blockquote> `64-64`: **LGTM! Clear documentation of the new option.** The option is well-documented with clear guidance on path resolution and a reference to detailed documentation. --- `114-121`: **LGTM! Well-structured example.** The example clearly demonstrates the new feature with a helpful note about path resolution. </blockquote></details> <details> <summary>src/NuGetLicense/Program.cs (6)</summary><blockquote> `119-122`: **LGTM! Option definition follows established patterns.** The new option is properly defined and documented, consistent with other command-line options. --- `132-149`: **LGTM! Proper introduction of IFileSystem abstraction.** The integration of `System.IO.Abstractions.FileSystem` improves testability and follows dependency injection principles. All dependent methods are consistently updated. --- `178-185`: **LGTM! Proper use of IFileSystem abstraction.** The method correctly uses the file system abstraction for file operations. --- `223-236`: **LGTM! Directory operations properly abstracted.** The method correctly uses the file system abstraction for directory checks and creation. --- `246-318`: **LGTM! Consistent application of IFileSystem abstraction.** All configuration reading methods are properly updated to use the file system abstraction, maintaining consistency and improving testability. --- `343-362`: **Address concerns from previous review.** A previous review identified several issues in this method that remain unaddressed: 1. **Line 351**: `Path.GetDirectoryName` can return null, but the code assumes a non-null result using the null-forgiving operator 2. **Lines 352, 353**: Null-forgiving operators on JSON deserialization without validation 3. **No error handling**: Missing validation for file existence, accessibility, and malformed JSON 4. **No test coverage**: The feature lacks unit or integration tests The previous review recommended: - Validate that `LicenseFileMappings` exists and is accessible before reading - Check that `GetDirectoryName` result is not null and throw a clear exception if it is - Perform null-checks on `JsonSerializer.Deserialize` with meaningful error messages - Add tests covering relative/absolute path resolution, cross-platform paths, and error cases </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
sensslen
force-pushed
the
simon/fix-invalid-license-paths
branch
from
c60ab83 to
aeb326c
Compare
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Fixes #290
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.