SP-3649-update-copyleft-inspection-rules #165
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Copyleft
participant ScanResultProcessor
CLI->>CLI: Parse --license-sources / -ls
CLI->>Copyleft: new Copyleft(license_sources=args.license_sources)
activate Copyleft
Copyleft->>Copyleft: license_sources = provided or DEFAULT_COPYLEFT_LICENSE_SOURCES
Copyleft->>ScanResultProcessor: new ScanResultProcessor(license_sources=self.license_sources)
activate ScanResultProcessor
ScanResultProcessor->>ScanResultProcessor: store license_sources
deactivate ScanResultProcessor
deactivate Copyleft
Note over ScanResultProcessor: During result processing
alt Filtering Mode (license_sources provided)
rect rgb(200,220,255)
ScanResultProcessor->>ScanResultProcessor: _select_licenses -> filter by source ∈ license_sources ∪ {'unknown'}
end
else Priority Mode (license_sources is None)
rect rgb(220,200,255)
ScanResultProcessor->>ScanResultProcessor: _select_licenses -> use source-priority selection
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
isasmendiagus
force-pushed
the
SP-3649-update-copyleft-inspection-rules
branch
from
ee2970e to
800decf
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
![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 (2)
src/scanoss/cli.py (1)
704-714: Consider: Clarify the append behavior in help text.The
action='append'allows users to specify--license-sourcesmultiple times (e.g.,--license-sources component_declared --license-sources license_file). While this works correctly, users might find it more intuitive to accept a comma-separated list instead, similar to how--includeand--excludework.Current behavior:
scanoss-py inspect copyleft -i results.json --license-sources component_declared --license-sources license_fileAlternative (if changed):
scanoss-py inspect copyleft -i results.json --license-sources component_declared,license_fileIf you prefer to keep the current approach, consider updating the help text to explicitly mention that the flag can be repeated multiple times.
src/scanoss/inspection/utils/scan_result_processor.py (1)
316-361: Consider: Enhance docstring to clarify dual-mode behavior.The
_select_licensesmethod implements two distinct behaviors (filtering vs. priority), which supports both the new license source filtering feature and maintains backward compatibility for other consumers. However, the dual-mode nature could be documented more explicitly.Consider enhancing the docstring to clarify:
- When filtering mode is used (license_sources provided) and its behavior
- When priority mode is used (license_sources is None) and its behavior
- That this design maintains backward compatibility for existing consumers like LicenseSummary
Current docstring is good but could be more explicit about the two paths through the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)src/scanoss/cli.py(3 hunks)src/scanoss/constants.py(1 hunks)src/scanoss/inspection/policy_check/scanoss/copyleft.py(4 hunks)src/scanoss/inspection/utils/scan_result_processor.py(3 hunks)tests/test_policy_inspect.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (1)
src/scanoss/inspection/utils/scan_result_processor.py (1)
ScanResultProcessor(53-412)
src/scanoss/inspection/utils/scan_result_processor.py (1)
src/scanoss/inspection/utils/license_utils.py (2)
LicenseUtil(52-132)init(68-105)
tests/test_policy_inspect.py (2)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
Copyleft(49-239)run(219-239)src/scanoss/inspection/policy_check/policy_check.py (2)
run(87-102)PolicyStatus(33-44)
🪛 GitHub Actions: Lint
src/scanoss/inspection/policy_check/scanoss/copyleft.py
[error] 25-25: Ruff I001: Import block is un-sorted or un-formatted. 2 fixable with the --fix option.
src/scanoss/cli.py
[error] 25-25: Ruff I001: Import block is un-sorted or un-formatted. 2 fixable with the --fix option.
⏰ 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)
- GitHub Check: build
🔇 Additional comments (10)
CHANGELOG.md (1)
10-10: LGTM! Clear changelog entry.The changelog entry accurately describes the new feature.
src/scanoss/constants.py (1)
20-22: LGTM! Well-defined constants.The constants are appropriately named and the default sources (
component_declared,license_file) represent the most reliable sources for copyleft license detection.src/scanoss/cli.py (2)
1769-1769: LGTM! Correct parameter passing.The
license_sourcesargument is correctly passed from CLI args to the Copyleft inspector.
67-68: Import sorting issue has been resolved.The ruff linter automatically de-duplicates, groups, and sorts imports based on isort settings. The
ruff check --fixcommand executed successfully, fixing the 1 import sorting error (I001) at this location with no remaining issues.src/scanoss/inspection/policy_check/scanoss/copyleft.py (3)
67-91: LGTM! Default behavior is well-implemented.The
license_sources or DEFAULT_COPYLEFT_LICENSE_SOURCESpattern (line 91) provides sensible defaults while allowing users to override. This ensures copyleft inspection always uses filtering mode with either user-specified sources or the default high-confidence sources.
99-100: LGTM! Parameter correctly passed to processor.The
license_sourcesis properly passed toScanResultProcessor, enabling the filtering behavior.
29-29: Import sorting issue resolved.The ruff linter has successfully fixed the import sorting error. The command found and corrected 1 error with no remaining issues.
src/scanoss/inspection/utils/scan_result_processor.py (2)
74-80: LGTM! Parameter added and stored correctly.The
license_sourcesparameter is properly integrated into the constructor and stored as an instance variable.
167-171: LGTM! License selection refactored correctly.The refactoring to use
_select_licensesmethod centralizes the license selection logic, making it easier to maintain and test.tests/test_policy_inspect.py (1)
393-645: LGTM! Excellent test coverage.The test suite is comprehensive and well-structured, covering:
- Default behavior and explicit None handling
- Individual source filtering (component_declared, license_file, file_header)
- Multiple source combinations
- Integration with existing filters (include/exclude)
- Different output formats (JSON, Markdown)
- Edge cases (no copyleft licenses, empty results)
The tests validate both the filtering logic and integration with the broader copyleft inspection workflow.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
68-68: Consider using a more specific type hint.The parameter uses a generic
listtype hint. Consider specifying the element type for better type safety and IDE support.Apply this diff to use a more specific type hint:
- license_sources: list = None, + license_sources: list[str] | None = None,Alternatively, if supporting older Python versions:
+from typing import List, Optional ... - license_sources: list = None, + license_sources: Optional[List[str]] = None,
83-83: Consider expanding the docstring for clarity.The docstring could provide more context about valid values and default behavior.
Consider this more descriptive docstring:
- :param license_sources: List of license sources to check + :param license_sources: List of license sources to check (e.g., 'component_declared', 'license_file', + 'file_header', 'scancode'). Defaults to DEFAULT_COPYLEFT_LICENSE_SOURCES + (component_declared and license_file) when None.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/scanoss/cli.py(4 hunks)src/scanoss/inspection/policy_check/scanoss/copyleft.py(4 hunks)tests/test_policy_inspect.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scanoss/cli.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_policy_inspect.py (2)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (2)
Copyleft(50-240)run(220-240)src/scanoss/inspection/policy_check/policy_check.py (2)
run(87-102)PolicyStatus(33-44)
⏰ 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)
- GitHub Check: build
🔇 Additional comments (6)
src/scanoss/inspection/policy_check/scanoss/copyleft.py (3)
29-30: LGTM!The import of
DEFAULT_COPYLEFT_LICENSE_SOURCESis clean and supports the new license source filtering feature.
92-92: LGTM!The default assignment correctly falls back to
DEFAULT_COPYLEFT_LICENSE_SOURCESwhenlicense_sourcesis not provided. Note that an empty list[]will also trigger the default, which is likely the intended behavior.
93-101: LGTM!The
license_sourcesparameter is correctly passed toScanResultProcessor, aligning with its constructor signature as documented in the relevant code snippets.tests/test_policy_inspect.py (3)
31-31: LGTM!The import of constants supports the comprehensive test coverage for license source filtering.
393-394: LGTM!The section header clearly delineates the new test suite for license source filtering functionality.
395-644: Excellent test coverage!The 11 new test methods provide comprehensive coverage of the license source filtering feature:
- Default and edge-case behavior (default sources, explicit None)
- Individual source filtering (component_declared, license_file, file_header)
- Combined source filtering (multiple sources, all valid sources)
- Integration with output formats (markdown)
- Integration with other filters (include/exclude)
- Edge cases (no copyleft results)
Each test is well-documented and follows a consistent, maintainable pattern.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
isasmendiagus
force-pushed
the
SP-3649-update-copyleft-inspection-rules
branch
from
9e2985c to
905c099
Compare
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
Summary by CodeRabbit
New Features
Tests