Extract SPDX license identifiers - #302
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Signed-off-by: Park Wonjae <wonjae.park@lge.com>
5ba8116 to
1662701
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: 1
🧹 Nitpick comments (3)
src/fosslight_source/cli.py (2)
454-461: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression tests for SPDX license merging.
Cover a matching
SourceItem, a license-only file, blank and non-string values, and a file that also has manifest licenses. These cases verify replacement, new-item creation, filtering, and precedence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` around lines 454 - 461, Add regression tests covering the SPDX merge logic around _get_or_append_source_item: verify matching SourceItem licenses are replaced, license-only files create new items, blank and non-string values are filtered, and SPDX licenses take precedence when manifest licenses also exist.
723-730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the
metadata_collectorreturn annotation.The new contract returns
(spdx_downloads, spdx_licenses, manifest_licenses), but Line 717 still declares-> dict. Use a tuple annotation for the threedict[str, list[str]]mappings.Proposed fix
-def metadata_collector(path_to_scan: str, excluded_files: set) -> dict: +def metadata_collector( + path_to_scan: str, excluded_files: set +) -> tuple[ + dict[str, list[str]], + dict[str, list[str]], + dict[str, list[str]], +]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/cli.py` around lines 723 - 730, Update the return annotation of metadata_collector to describe its three-value tuple result: spdx_downloads, spdx_licenses, and manifest_licenses, each typed as dict[str, list[str]].src/fosslight_source/run_spdx_extractor.py (1)
26-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the exception handler.
except Exceptionhides unexpected defects and returns empty metadata. A decoding or mapping error can therefore look like a file with no SPDX metadata. Catch expectedOSError,ValueError, andUnicodeDecodeErrorcases. Log unexpected failures with traceback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_source/run_spdx_extractor.py` around lines 26 - 27, Update the exception handling in the SPDX extraction flow to catch only expected OSError, ValueError, and UnicodeDecodeError failures while preserving the empty-metadata fallback for those cases. Add a separate unexpected-error path that logs the failure with traceback instead of silently treating it as missing metadata, using the existing logger and extraction function context.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fosslight_source/cli.py`:
- Around line 397-399: The merge_results signature uses unsafe mutable defaults
for scancode_result, scanoss_result, spdx_downloads, spdx_licenses, and
manifest_licenses. Change these defaults to None and initialize each collection
inside merge_results before any mutation or access, while preserving the
existing behavior for callers that provide explicit results.
---
Nitpick comments:
In `@src/fosslight_source/cli.py`:
- Around line 454-461: Add regression tests covering the SPDX merge logic around
_get_or_append_source_item: verify matching SourceItem licenses are replaced,
license-only files create new items, blank and non-string values are filtered,
and SPDX licenses take precedence when manifest licenses also exist.
- Around line 723-730: Update the return annotation of metadata_collector to
describe its three-value tuple result: spdx_downloads, spdx_licenses, and
manifest_licenses, each typed as dict[str, list[str]].
In `@src/fosslight_source/run_spdx_extractor.py`:
- Around line 26-27: Update the exception handling in the SPDX extraction flow
to catch only expected OSError, ValueError, and UnicodeDecodeError failures
while preserving the empty-metadata fallback for those cases. Add a separate
unexpected-error path that logs the failure with traceback instead of silently
treating it as missing metadata, using the existing logger and extraction
function context.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e873c03b-7178-49e6-ab2a-b9429e0d98f7
📒 Files selected for processing (2)
src/fosslight_source/cli.pysrc/fosslight_source/run_spdx_extractor.py
| scancode_result: list = [], scanoss_result: list = [], spdx_downloads: dict = {}, spdx_licenses: dict = {}, | ||
| path_to_scan: str = "", run_kb: bool = False, manifest_licenses: dict = {}, | ||
| excluded_files: set = None, hide_progress: bool = False, kb_url: str = "", kb_token: str = "", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)cli\.py$' || true
echo "== function outline around merge_results =="
if [ -f src/fosslight_source/cli.py ]; then
ast-grep outline src/fosslight_source/cli.py --match merge_results --view expanded || true
echo "== line count =="
wc -l src/fosslight_source/cli.py
echo "== relevant section =="
sed -n '340,460p' src/fosslight_source/cli.py
fi
echo "== mutable defaults / merge_results signatures and usages =="
rg -n "def merge_results|merge_results\(" src/fosslight_source -S || trueRepository: fosslight/fosslight_source_scanner
Length of output: 6247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports =="
sed -n '1,80p' src/fosslight_source/cli.py
echo "== merge_results tail =="
sed -n '420,495p' src/fosslight_source/cli.py
echo "== callers =="
sed -n '640,685p' src/fosslight_source/cli.py
echo "== all direct merge_results references in repo =="
rg -n "merge_results\(" . -S --glob '!*.git/**' --glob '!node_modules/**' || true
echo "== deterministic default behavior for similar function =="
python3 - <<'PY'
def f(x=[]):
x.append(1)
return x
print(f(), f())
PYRepository: fosslight/fosslight_source_scanner
Length of output: 9970
Remove the mutable defaults from merge_results.
merge_results mutuates scancode_result with append, and the current CLI call path always passes an explicit result; the defaults still make the helper unsafe for future calls or imports that omit it. The dict defaults with spdx_downloads, spdx_licenses, and manifest_licenses follow the same pattern. Use None defaults and initialize each collection inside merge_results.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 397-397: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 397-397: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 397-397: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 397-397: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 398-398: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 399-399: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fosslight_source/cli.py` around lines 397 - 399, The merge_results
signature uses unsafe mutable defaults for scancode_result, scanoss_result,
spdx_downloads, spdx_licenses, and manifest_licenses. Change these defaults to
None and initialize each collection inside merge_results before any mutation or
access, while preserving the existing behavior for callers that provide explicit
results.
Source: Linters/SAST tools
|
협의에 의해 반영하지 않습니다. |
1 participant
Summary by CodeRabbit