Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded get_license_expression_spdx to normalize trimmed license expressions to SPDX via licensedcode.cache.build_spdx_license_expression; parsing_scancode_32_later now substitutes found license strings with the SPDX expression when available. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Helper as get_license_expression_spdx
participant LicensedCode as licensedcode.cache
Parser->>Helper: pass found_lic
Helper->>LicensedCode: build_spdx_license_expression(trim(found_lic))
LicensedCode-->>Helper: SPDX expression or error
Helper-->>Parser: SPDX expression or empty string
alt SPDX non-empty
Parser->>Parser: replace found_lic with SPDX expression
else SPDX empty
Parser->>Parser: keep original found_lic
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 210-220: In get_license_expression_spdx, catch exceptions the same
way other handlers do and log the failure with logger.debug including the
exception info; also guard against build_spdx_license_expression returning None
by checking the result before calling str() and return "" when result is None or
starts with "LicenseRef-". Use the existing function name
get_license_expression_spdx and the called symbol build_spdx_license_expression
and follow the file's logger.debug pattern for consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fosslight_source/_parsing_scancode_file_item.py (1)
283-284:license_expression_spdxis reused for a semantically different variable at line 310.Line 283 uses
license_expression_spdxfor a per-match SPDX lookup; line 310 reuses the same name for the file-leveldetected_license_expression_spdxfield. While there's no functional conflict (different scopes in the loop), the shadowing reduces clarity.♻️ Proposed rename at line 283
- license_expression_spdx = get_license_expression_spdx(found_lic) - found_lic = license_expression_spdx if license_expression_spdx else found_lic + matched_lic_spdx = get_license_expression_spdx(found_lic) + found_lic = matched_lic_spdx if matched_lic_spdx else found_lic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fosslight_source/_parsing_scancode_file_item.py` around lines 283 - 284, The local variable license_expression_spdx used when processing each match should be renamed to avoid shadowing the file-level detected_license_expression_spdx; update the per-match variable (where get_license_expression_spdx(found_lic) is called and assigned back into found_lic) to a clearer name like match_license_expression_spdx or license_expression_match_spdx, and use that renamed identifier in the subsequent assignment to found_lic so the later file-level detected_license_expression_spdx remains unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 216-218: The current guard is ineffective because result is always
overwritten; change the logic so that when result_obj is falsy/None you
immediately return an empty string (or set result = "" and skip conversion)
instead of executing result = str(result_obj); specifically update the code
around result_obj (the block using build_spdx_license_expression / result_obj)
to: if not result_obj: return "" (or if you must keep result variable: if not
result_obj: result = "" else: result = str(result_obj)) so that "None" cannot be
produced as a license string.
- Around line 213-223: The function get_license_expression_spdx incorrectly
handles a falsy result_obj: it sets result = "" then immediately overwrites with
str(result_obj), which yields "None" when result_obj is None; update the logic
to check result_obj and return "" early (or use a try-else) before converting to
string, e.g. call build_spdx_license_expression(license_expression.strip()), if
result_obj is falsy return "" immediately, otherwise set result =
str(result_obj) and then apply the LicenseRef- check; ensure exception handling
remains to return "" on errors.
---
Nitpick comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 283-284: The local variable license_expression_spdx used when
processing each match should be renamed to avoid shadowing the file-level
detected_license_expression_spdx; update the per-match variable (where
get_license_expression_spdx(found_lic) is called and assigned back into
found_lic) to a clearer name like match_license_expression_spdx or
license_expression_match_spdx, and use that renamed identifier in the subsequent
assignment to found_lic so the later file-level detected_license_expression_spdx
remains unambiguous.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fosslight_source/_parsing_scancode_file_item.py`:
- Around line 210-220: The function get_license_expression_spdx must handle
build_spdx_license_expression returning None before calling regex.match; modify
get_license_expression_spdx to check if result is None (or falsy) and return ""
immediately prior to calling regex.match(result), and replace the bare except
with a narrower exception handling (or rethrow) so TypeError from regex.match
won't be silently swallowed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of change