Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCoverItem now tracks a finish_time and computes a running_time string from start_time and finish_time, replacing the previous start_time export with running_time under a relabeled "Running time" key. New helper functions format timestamps and durations, and ScannerItem gains a method to set the cover's finish time. ChangesRunning Time Tracking
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant ScannerItem
participant CoverItem
participant JSONOutput
ScannerItem->>ScannerItem: set_cover_finish_time(finish_time)
ScannerItem->>CoverItem: set_finish_time(finish_time)
CoverItem->>CoverItem: _format_running_time(start_time, finish_time)
CoverItem-->>ScannerItem: running_time updated
CoverItem->>JSONOutput: get_print_json() emits running_time under "Running Time"
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/fosslight_util/cover.py (1)
7-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit test coverage for the new helper functions.
_format_display_time,_format_duration_compact,format_running_time, anddump_result_logare new, pure, easily-testable functions with several edge cases (missing finish_time, zero-duration, hour/minute boundaries, quote-stripping). No tests are included in this PR.🤖 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_util/cover.py` around lines 7 - 46, Add unit tests for the new pure helpers in cover.py: _format_display_time, _format_duration_compact, format_running_time, and dump_result_log. Cover key edge cases such as zero duration, hour/minute boundaries, and the Running time quote-stripping behavior in dump_result_log. Also include a case for format_running_time with a missing or invalid finish_time if that path is expected to be handled, and verify the exact formatted strings returned by each function.
🤖 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.
Nitpick comments:
In `@src/fosslight_util/cover.py`:
- Around line 7-46: Add unit tests for the new pure helpers in cover.py:
_format_display_time, _format_duration_compact, format_running_time, and
dump_result_log. Cover key edge cases such as zero duration, hour/minute
boundaries, and the Running time quote-stripping behavior in dump_result_log.
Also include a case for format_running_time with a missing or invalid
finish_time if that path is expected to be handled, and verify the exact
formatted strings returned by each function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c772a855-61b5-4fcd-8efd-5c11d43db11e
📒 Files selected for processing (2)
src/fosslight_util/cover.pysrc/fosslight_util/oss_item.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/fosslight_util/cover.py (4)
67-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile regex post-processing of YAML output.
Stripping quotes from the
"Running time"value with a regex afteryaml.safe_dumpis brittle: it depends on the exact serialized quote style yaml chooses and on the value never containing a": "pattern (colon followed by space) which would otherwise require quoting to remain valid YAML. If the running-time format changes later (e.g. adds a colon-space sequence), this could silently produce invalid or misleading YAML.Consider using a custom YAML representer or
default_styleto control quoting for this key deterministically instead of string-level regex surgery.🤖 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_util/cover.py` around lines 67 - 70, The post-processing in dump_result_log is brittle because it rewrites yaml.safe_dump output with a regex to unquote the Running time field. Replace that string-level manipulation with deterministic YAML formatting for that key, such as a custom representer or controlled scalar style in dump_result_log, so the Running time value is emitted correctly without relying on the serialized quote style.
88-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMutable default argument (
exclude_path=[]).Flagged by Ruff (B006). A shared mutable list as a default argument can lead to subtle bugs if it's ever mutated in place across calls.
🛠️ Proposed fix
- def __init__(self, tool_name="", start_time="", input_path="", comment="", exclude_path=[], simple_mode=True, + def __init__(self, tool_name="", start_time="", input_path="", comment="", exclude_path=None, simple_mode=True, finish_time=""): + if exclude_path is None: + exclude_path = [] if simple_mode:🤖 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_util/cover.py` around lines 88 - 89, The constructor in Cover.__init__ uses a shared mutable default for exclude_path, which can leak state across instances. Change the default to None, then initialize a fresh list inside __init__ when no value is provided, keeping the existing parameter name and behavior intact.Source: Linters/SAST tools
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
start_time_keynow stores a running-time label, not a start time.
start_time_keyis relabeled to"Running time"andget_print_jsonassignsself.running_timeto it, so the attribute name no longer matches its purpose. Renaming to something likerunning_time_keywould improve clarity for future maintainers.Also applies to: 146-146
🤖 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_util/cover.py` at line 75, The naming in cover.py is misleading because start_time_key now holds the “Running time” label and is used by get_print_json for self.running_time. Rename this identifier to something like running_time_key wherever it is defined and referenced, and update the related use in get_print_json so the symbol name matches its purpose.
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate timestamp-to-display formatting logic.
_format_display_timeandformat_running_timeboth parse the UTC timestamp and convert/format it to_DISPLAY_TZindependently. Consider reusing_format_display_timeinsideformat_running_timeto avoid drift if the display format ever changes.♻️ Proposed refactor
def format_running_time(start_time, finish_time): start_utc = _parse_utc_timestamp(start_time) finish_utc = _parse_utc_timestamp(finish_time) total_seconds = int((finish_utc - start_utc).total_seconds()) - start_display = start_utc.astimezone(_DISPLAY_TZ).strftime('%Y%m%d_%H:%M:%S') - finish_display = finish_utc.astimezone(_DISPLAY_TZ).strftime('%Y%m%d_%H:%M:%S') + start_display = _format_display_time(start_time) + finish_display = _format_display_time(finish_time) duration = _format_duration_compact(total_seconds) return f'{start_display} ~ {finish_display} {duration}'Also applies to: 55-65
🤖 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_util/cover.py` around lines 38 - 39, The timestamp parsing and display formatting logic is duplicated between _format_display_time and format_running_time, so update format_running_time to reuse _format_display_time instead of repeating the UTC parse, timezone conversion, and strftime formatting. Keep the behavior consistent by funneling all display-time formatting through the shared helper to avoid future drift if the format changes.
🤖 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_util/cover.py`:
- Around line 42-52: The _format_duration_compact helper in cover.py can format
negative total_seconds as a misleading positive duration because divmod floors
negative values; add an explicit guard for finish_time earlier than start_time
(or any negative total_seconds) and decide on a safe fallback such as zeroing
the duration, clamping to 0, or raising/returning an invalid marker before
building the duration_items list. Keep the fix localized to
_format_duration_compact and any callers that compute total_seconds so inverted
ranges are handled consistently in both cover output paths.
---
Nitpick comments:
In `@src/fosslight_util/cover.py`:
- Around line 67-70: The post-processing in dump_result_log is brittle because
it rewrites yaml.safe_dump output with a regex to unquote the Running time
field. Replace that string-level manipulation with deterministic YAML formatting
for that key, such as a custom representer or controlled scalar style in
dump_result_log, so the Running time value is emitted correctly without relying
on the serialized quote style.
- Around line 88-89: The constructor in Cover.__init__ uses a shared mutable
default for exclude_path, which can leak state across instances. Change the
default to None, then initialize a fresh list inside __init__ when no value is
provided, keeping the existing parameter name and behavior intact.
- Line 75: The naming in cover.py is misleading because start_time_key now holds
the “Running time” label and is used by get_print_json for self.running_time.
Rename this identifier to something like running_time_key wherever it is defined
and referenced, and update the related use in get_print_json so the symbol name
matches its purpose.
- Around line 38-39: The timestamp parsing and display formatting logic is
duplicated between _format_display_time and format_running_time, so update
format_running_time to reuse _format_display_time instead of repeating the UTC
parse, timezone conversion, and strftime formatting. Keep the behavior
consistent by funneling all display-time formatting through the shared helper to
avoid future drift if the format changes.
🪄 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: 609a6472-d235-4999-81a0-48768e7d666c
📒 Files selected for processing (2)
src/fosslight_util/cover.pysrc/fosslight_util/oss_item.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fosslight_util/oss_item.py
1 participant
Description
Summary by CodeRabbit
New Features
Bug Fixes