Strip root path from local package output and add local paths to comments for result file#312
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds relative-path scanner logging, treats PyPI ChangesDependency info and logging enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (2)
src/fosslight_dependency/run_dependency_scanner.py (1)
181-181: ⚡ Quick winFix variable shadowing flagged by static analysis.
The loop variable
pmshadows the function parameterpm, reducing readability and triggering a Ruff warning (B020).♻️ Proposed fix
- for pm, dir_dict in pm.items(): - log_lines.append(f"- {status} {pm}:") + for pm_name, dir_dict in pm.items(): + log_lines.append(f"- {status} {pm_name}:")🤖 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_dependency/run_dependency_scanner.py` at line 181, The loop variable in "for pm, dir_dict in pm.items()" shadows the function parameter named pm; rename the loop key to a distinct identifier (e.g., "for pm_name, dir_dict in pm.items()" or "for manager, dir_dict in pm.items()") and update any references inside that loop to the new name so the parameter "pm" remains accessible and the Ruff B020 warning is resolved.src/fosslight_dependency/package_manager/Pypi.py (1)
421-425: ⚖️ Poor tradeoffConsider using
urllib.parse+urllib.requestfor more robust and maintainable file:// URL handling.Per PEP 610, pip standardizes on
file:///absolute/pathformat for local dependencies (three slashes for absolute paths). The current regex-based approach works correctly for this format. However, using Python's standard library provides better maintainability and handles edge cases:from urllib.parse import urlparse, unquote from urllib.request import url2pathname if direct_url_str.startswith('file://'): is_local = True parsed = urlparse(direct_url_str) local_path = url2pathname(unquote(parsed.path)) local_path = os.path.normpath(local_path)This approach is more explicit and would handle variations if the source format changes in the future. Note:
urllibis already imported elsewhere in the codebase (Go.py), so this adds no new dependencies.🤖 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_dependency/package_manager/Pypi.py` around lines 421 - 425, Replace the regex-based file:// handling for direct_url_str with urllib utilities: when direct_url_str startswith 'file://', set is_local True, parse the URL using urlparse and unquote, convert the parsed path to a native filesystem path with url2pathname, then normalize with os.path.normpath instead of the current re.sub approach; update the logic around the variables direct_url_str, is_local, and local_path (and import urlparse/unquote/url2pathname where needed) to ensure robust PEP 610 file:// handling.
🤖 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_dependency/package_manager/Pypi.py`:
- Around line 445-446: When adding a local path, do not overwrite
oss_item.comment; instead preserve existing dependency metadata by appending the
local path info. Update the logic around oss_item.comment and local_path_comment
so that if oss_item.comment is non-empty you set oss_item.comment =
f"{oss_item.comment} (local: {local_path_comment})", otherwise set it to
local_path_comment (or "(local: ...)" as desired); apply this change where
oss_item.comment is assigned so root/direct/transitive markers are retained.
In `@src/fosslight_dependency/run_dependency_scanner.py`:
- Around line 185-192: Wrap the os.path.relpath call in a try/except to catch
ValueError from cross-drive paths: in the block that computes rel_path (the code
referencing base_dir, rel_path, path and appending to log_lines), catch
ValueError around os.path.relpath and in the except branch fall back to using
the original path (or os.path.abspath(path)) when building the log line so the
logger never crashes on Windows drives mismatch.
- Line 292: The variable base_path is initialized too early (base_path = '')
which can leave it empty if an exception occurs before it is set and leads to
blank "Analyzed path:" logs in the finally block; to fix, remove the early
initialization and instead assign base_path only after input_dir validation (the
code that validates input_dir in run_dependency_scanner.py) or immediately
before the try block that uses it so it cannot remain empty, and also remove the
duplicate assignment mentioned in the proposed fix so found_package_manager and
the finally block always see a valid base_path value.
---
Nitpick comments:
In `@src/fosslight_dependency/package_manager/Pypi.py`:
- Around line 421-425: Replace the regex-based file:// handling for
direct_url_str with urllib utilities: when direct_url_str startswith 'file://',
set is_local True, parse the URL using urlparse and unquote, convert the parsed
path to a native filesystem path with url2pathname, then normalize with
os.path.normpath instead of the current re.sub approach; update the logic around
the variables direct_url_str, is_local, and local_path (and import
urlparse/unquote/url2pathname where needed) to ensure robust PEP 610 file://
handling.
In `@src/fosslight_dependency/run_dependency_scanner.py`:
- Line 181: The loop variable in "for pm, dir_dict in pm.items()" shadows the
function parameter named pm; rename the loop key to a distinct identifier (e.g.,
"for pm_name, dir_dict in pm.items()" or "for manager, dir_dict in pm.items()")
and update any references inside that loop to the new name so the parameter "pm"
remains accessible and the Ruff B020 warning is resolved.
🪄 Autofix (Beta)
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
Run ID: 34685cd3-2d5f-43f7-8001-dcfd0ad9d361
📒 Files selected for processing (2)
src/fosslight_dependency/package_manager/Pypi.pysrc/fosslight_dependency/run_dependency_scanner.py
woocheol-lge
force-pushed
the
log_test
branch
3 times, most recently
from
1de988a to
0412126
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/fosslight_dependency/run_dependency_scanner.py (1)
185-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle cross-drive
relpathfailure on Windows.Line 186 can raise
ValueErrorwhenpathandbase_dirare on different drives, which can break logging. Add a safe fallback in this block.🔧 Proposed fix
if base_dir: - rel_path = os.path.relpath(path, base_dir) - if rel_path == '.': - log_lines.append(f" {file_list}") - else: - log_lines.append(f" {rel_path}{os.sep}{file_list}") + try: + rel_path = os.path.relpath(path, base_dir) + if rel_path == '.': + log_lines.append(f" {file_list}") + else: + log_lines.append(f" {rel_path}{os.sep}{file_list}") + except ValueError: + log_lines.append(f" {path}: {file_list}") else: log_lines.append(f" {path}: {file_list}")#!/bin/bash # Verify whether os.path.relpath is currently unguarded in print_package_info. rg -n -C4 'if base_dir:|os\.path\.relpath\(path, base_dir\)|except ValueError' src/fosslight_dependency/run_dependency_scanner.py🤖 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_dependency/run_dependency_scanner.py` around lines 185 - 190, The relpath call in print_package_info can raise ValueError on Windows when path and base_dir are on different drives; wrap the os.path.relpath(path, base_dir) call in a try/except ValueError and on exception set rel_path to path (or os.path.normpath(path)) so logging continues; update the block that assigns rel_path and appends to log_lines (referencing variables base_dir, path, rel_path, file_list and function print_package_info) to use this safe fallback.
🧹 Nitpick comments (1)
src/fosslight_dependency/package_manager/Pub.py (1)
70-71: 💤 Low valueConsider catching more specific exceptions.
Catching bare
Exceptionis broad. For clarity, consider catching specific exception types such asyaml.YAMLError,OSError, orKeyError. Since this is supplementary logic that gracefully degrades, the current approach is acceptable but less precise.♻️ Proposed refinement
- except Exception as e: + except (yaml.YAMLError, OSError, KeyError, TypeError) as e: logger.debug(f'Failed to supplement pkg_details from pubspec.lock: {e}')🤖 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_dependency/package_manager/Pub.py` around lines 70 - 71, Replace the broad "except Exception as e:" that logs "Failed to supplement pkg_details from pubspec.lock" with handlers for the likely error types—e.g., catch yaml.YAMLError for YAML parsing issues, OSError for file I/O problems, and KeyError (or ValueError) for missing keys/invalid data—then call logger.debug including the exception details (the existing message + the exception). Ensure yaml.YAMLError is imported if not already and keep a final generic except Exception only if you want a last-resort fallback, but prefer the specific handlers around the code that reads/parses pubspec.lock and populates pkg_details (the block that currently logs via logger.debug).
🤖 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_dependency/package_manager/Pub.py`:
- Around line 47-48: The docstring for supplement_pkg_details_from_lock is
written in Korean; update it to English to match project conventions by
replacing the existing Korean string with a concise English description (e.g.,
"Populate pkg_details with path/git source package locations from
pubspec.lock"). Ensure the new docstring remains a short triple-quoted string
immediately above def supplement_pkg_details_from_lock and preserves the
original intent about supplementing pkg_details from pubspec.lock for path/git
packages.
- Around line 49-51: The code currently looks up 'pubspec.lock' using the CWD;
change it to resolve the lock file path relative to the instance input directory
so it doesn't depend on process CWD. In the method containing "lock_file =
'pubspec.lock'" (e.g., parse_direct_dependencies / Pub class methods), replace
that static name with a path built from self.input_dir (use
os.path.join(self.input_dir, 'pubspec.lock')) and then check os.path.exists on
that resolved path and use that path for subsequent file access.
- Around line 61-64: The code currently overwrites self.pkg_details[dep_key]
when adding path or git info, losing existing fields; instead, merge/update the
existing dict: locate the blocks that set self.pkg_details[dep_key] (the path
branch using pkg_info.get('description') and the git branch that sets
'source':'git' and 'git'/'path'), ensure you call
self.pkg_details.setdefault(dep_key, {}) and then update only the relevant keys
(e.g., set 'source' and 'path' or set 'source' and 'git') via dict.update or
assignment so existing fields on self.pkg_details[dep_key] are preserved.
- Around line 54-55: yaml.safe_load(f) can return None for an empty file,
causing lock_data.get(...) to fail; after calling yaml.safe_load (the variable
lock_data used before computing packages) ensure lock_data is a dict (e.g.,
replace lock_data = yaml.safe_load(f) with lock_data = yaml.safe_load(f) or {}
or add an explicit None check) so that packages = lock_data.get('packages', {})
won't raise an AttributeError.
---
Duplicate comments:
In `@src/fosslight_dependency/run_dependency_scanner.py`:
- Around line 185-190: The relpath call in print_package_info can raise
ValueError on Windows when path and base_dir are on different drives; wrap the
os.path.relpath(path, base_dir) call in a try/except ValueError and on exception
set rel_path to path (or os.path.normpath(path)) so logging continues; update
the block that assigns rel_path and appends to log_lines (referencing variables
base_dir, path, rel_path, file_list and function print_package_info) to use this
safe fallback.
---
Nitpick comments:
In `@src/fosslight_dependency/package_manager/Pub.py`:
- Around line 70-71: Replace the broad "except Exception as e:" that logs
"Failed to supplement pkg_details from pubspec.lock" with handlers for the
likely error types—e.g., catch yaml.YAMLError for YAML parsing issues, OSError
for file I/O problems, and KeyError (or ValueError) for missing keys/invalid
data—then call logger.debug including the exception details (the existing
message + the exception). Ensure yaml.YAMLError is imported if not already and
keep a final generic except Exception only if you want a last-resort fallback,
but prefer the specific handlers around the code that reads/parses pubspec.lock
and populates pkg_details (the block that currently logs via logger.debug).
🪄 Autofix (Beta)
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
Run ID: 60c9e3d4-38e7-4237-884f-6a4524056918
📒 Files selected for processing (3)
src/fosslight_dependency/package_manager/Pub.pysrc/fosslight_dependency/package_manager/Pypi.pysrc/fosslight_dependency/run_dependency_scanner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fosslight_dependency/package_manager/Pypi.py
…d add local paths to comments for scanner info and dep sheet Signed-off-by: woocheol <jayden6659@gmail.com>
dd-jy
approved these changes
May 19, 2026
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
2 participants
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
Strip root path from local package output and add local paths to comments for result file
Summary by CodeRabbit
New Features
Improvements