Skip to content

Refactor ratchet coverage action#21

Merged
leynos merged 3 commits intomainfrom
codex/extract-and-convert-bash-scripts-to-python
Jul 5, 2025
Merged

Refactor ratchet coverage action#21
leynos merged 3 commits intomainfrom
codex/extract-and-convert-bash-scripts-to-python

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented Jul 5, 2025

Summary

  • migrate shell steps in ratchet-coverage to Python scripts
  • call scripts via uv run --script
  • parse arguments with shlex and expose GITHUB_OUTPUT via Typer
  • extract coverage parsing into a helper function
  • factor out baseline reading helper

Testing

  • python3 -m py_compile scripts/ratchet_coverage/*.py

https://chatgpt.com/codex/tasks/task_e_6869726f4fb0832291bae91bc5ea88a4

Summary by Sourcery

Refactor the ratchet-coverage GitHub Action to replace shell steps with modular Python scripts invoked via uv run, enhancing coverage parsing and baseline management.

Enhancements:

  • Migrate coverage installation, execution, and ratchet logic from shell to Python scripts invoked via uv run
  • Extract coverage percent parsing and baseline reading into reusable helper functions
  • Leverage Typer for robust argument and environment variable parsing in coverage scripts

CI:

  • Update action.yml to call the new Python scripts for installing cargo-llvm-cov, running coverage, and enforcing coverage ratchet

Tests:

  • Add a compile check to ensure the new Python scripts are syntactically valid

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Jul 5, 2025

Reviewer's Guide

This PR refactors the ratchet coverage GitHub Action by migrating inline shell logic into dedicated Python scripts invoked via uv’s script runner, using Typer for argument parsing and reusable helpers for coverage extraction and baseline management.

Sequence diagram for refactored ratchet coverage GitHub Action workflow

sequenceDiagram
    participant GitHubActions as GitHub Actions Runner
    participant InstallScript as install_cargo_llvm_cov.py
    participant RunCoverage as run_coverage.py
    participant RatchetCoverage as ratchet_coverage.py

    GitHubActions->>InstallScript: uv run --script install_cargo_llvm_cov.py
    InstallScript->>InstallScript: cargo install cargo-llvm-cov

    GitHubActions->>RunCoverage: uv run --script run_coverage.py
    RunCoverage->>RunCoverage: cargo llvm-cov --summary-only $ARGS
    RunCoverage->>RunCoverage: extract_percent(output)
    RunCoverage->>GitHubActions: Write percent to $GITHUB_OUTPUT

    GitHubActions->>RatchetCoverage: uv run --script ratchet_coverage.py (CURRENT_PERCENT env)
    RatchetCoverage->>RatchetCoverage: read_baseline(baseline_file)
    RatchetCoverage->>RatchetCoverage: Compare current vs baseline
    RatchetCoverage->>RatchetCoverage: Write new baseline if not decreased
Loading

Class diagram for new ratchet_coverage Python scripts

classDiagram
    class install_cargo_llvm_cov {
        +main()
    }
    class run_coverage {
        +main(args: str, github_output: Path)
        +extract_percent(output: str) str
    }
    class ratchet_coverage {
        +main(baseline_file: Path, current: float)
        +read_baseline(file: Path) float
    }
    install_cargo_llvm_cov <.. run_coverage : uses plumbum.cmd.cargo
    install_cargo_llvm_cov <.. ratchet_coverage : uses plumbum.cmd.cargo
    run_coverage <.. ratchet_coverage : shares Typer usage
    run_coverage <.. ratchet_coverage : both use Typer CLI
    run_coverage <.. ratchet_coverage : both use Path
Loading

Flow diagram for the new ratchet coverage action process

flowchart TD
    A[Start GitHub Action] --> B[Install cargo-llvm-cov via Python script]
    B --> C[Run coverage via Python script]
    C --> D[Parse and write percent to GITHUB_OUTPUT]
    D --> E[Ratchet coverage check via Python script]
    E -->|Coverage decreased| F[Fail Action]
    E -->|Coverage OK| G[Update baseline file]
Loading

File-Level Changes

Change Details Files
Migrate GitHub Action shell steps to Python script invocations
  • Replaced inline ‘Install cargo-llvm-cov’ command with Python script call
  • Swapped shell ‘Run coverage’ block for run_coverage.py invocation
  • Replaced ratchet coverage shell logic with ratchet_coverage.py and passed CURRENT_PERCENT via env
.github/actions/ratchet-coverage/action.yml
Introduce run_coverage.py to encapsulate coverage collection and parsing
  • Define Typer CLI to accept INPUT_ARGS and GITHUB_OUTPUT paths
  • Use plumbum to run cargo llvm-cov and capture summary output
  • Extract and validate percentage via regex helper
  • Append percent to GITHUB_OUTPUT file
scripts/ratchet_coverage/run_coverage.py
Add ratchet_coverage.py to compare and persist coverage baseline
  • Implement read_baseline helper to safely load float or default to 0
  • Compare current percent against baseline, exit on regression
  • Create parent directories and write formatted baseline file
  • Expose baseline path and current percent via Typer Options
scripts/ratchet_coverage/ratchet_coverage.py
Add install_cargo_llvm_cov.py to modularize installation step
  • Provide a Typer-based script to run cargo install cargo-llvm-cov via plumbum
  • Simplify installation step in action.yml
scripts/ratchet_coverage/install_cargo_llvm_cov.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jul 5, 2025

Summary by CodeRabbit

  • Chores
    • Updated the coverage workflow to use Python scripts for installing tools, running coverage, and enforcing coverage thresholds.
    • Introduced new scripts to automate installation, run coverage analysis, and manage coverage baselines, improving reliability and maintainability of the coverage process.

Summary by CodeRabbit

  • Chores
    • Updated the coverage workflow to use Python scripts for installing, running, and enforcing code coverage checks, replacing previous shell commands. This streamlines and standardises the coverage process within the project’s automation.

Walkthrough

The ratchet coverage GitHub Action has been refactored to replace inline shell script steps with dedicated Python scripts for installing cargo-llvm-cov, running coverage analysis, and enforcing coverage ratcheting. The control flow and logic remain consistent, but operational details are now handled by Python scripts using typer and plumbum.

Changes

File(s) Change Summary
.github/actions/ratchet-coverage/action.yml Replaced shell script steps with invocations of new Python scripts for install, run, and ratchet steps
scripts/ratchet_coverage/install_cargo_llvm_cov.py Added script to install cargo-llvm-cov using plumbum and typer
scripts/ratchet_coverage/ratchet_coverage.py Added script to compare and update coverage baseline, enforcing ratcheting logic
scripts/ratchet_coverage/run_coverage.py Added script to run coverage, extract percentage, and output result

Sequence Diagram(s)

sequenceDiagram
    participant GitHub Action
    participant install_cargo_llvm_cov.py
    participant run_coverage.py
    participant ratchet_coverage.py

    GitHub Action->>install_cargo_llvm_cov.py: Install cargo-llvm-cov
    install_cargo_llvm_cov.py-->>GitHub Action: Installation complete

    GitHub Action->>run_coverage.py: Run coverage analysis
    run_coverage.py-->>GitHub Action: Output coverage percentage

    GitHub Action->>ratchet_coverage.py: Enforce ratchet with coverage percentage
    ratchet_coverage.py-->>GitHub Action: Update baseline or fail on regression
Loading

Possibly related PRs

Poem

A rabbit hopped through coverage land,
Swapping shell for Python, just as planned.
With typer and plumbum scripts in tow,
The Action’s checks now smoothly flow.
“No regressions!” the bunny cheers,
As code stays strong through all the years.
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Post Copyable Unit Tests in a Comment
  • Commit Unit Tests in branch codex/extract-and-convert-bash-scripts-to-python

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @leynos - I've reviewed your changes - here's some feedback:

  • The three Python scripts share identical shebang headers and dependency declarations—consider extracting those and any common Typer options into a shared module or template to reduce duplication and keep them in sync.
  • Wrap calls to plumbum (e.g. cargo install and cargo llvm-cov) in try/except or explicitly check their return codes so you can emit clearer, contextual error messages instead of relying on generic exceptions.
  • Before comparing current and baseline, normalize both floats to the same precision (e.g. round to two decimals) to avoid failures caused by minor floating‐point rounding differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The three Python scripts share identical shebang headers and dependency declarations—consider extracting those and any common Typer options into a shared module or template to reduce duplication and keep them in sync.
- Wrap calls to plumbum (e.g. `cargo install` and `cargo llvm-cov`) in try/except or explicitly check their return codes so you can emit clearer, contextual error messages instead of relying on generic exceptions.
- Before comparing `current` and `baseline`, normalize both floats to the same precision (e.g. round to two decimals) to avoid failures caused by minor floating‐point rounding differences.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread scripts/ratchet_coverage/run_coverage.py Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between debf7d6 and 149aa78.

📒 Files selected for processing (4)
  • .github/actions/ratchet-coverage/action.yml (2 hunks)
  • scripts/ratchet_coverage/install_cargo_llvm_cov.py (1 hunks)
  • scripts/ratchet_coverage/ratchet_coverage.py (1 hunks)
  • scripts/ratchet_coverage/run_coverage.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`.github/actions/*/{action.yml,README.md,CHANGELOG.md,src/,tests/}`: Each action...

.github/actions/*/{action.yml,README.md,CHANGELOG.md,src/,tests/}: Each action must have its own directory under .github/actions/, containing action.yml, README.md, src/, tests/, and CHANGELOG.md

📄 Source: CodeRabbit Inference Engine (AGENTS.md)

List of files the instruction was applied to:

  • .github/actions/ratchet-coverage/action.yml
`.github/actions/*/action.yml`: Fill in action.yml with every input and output; ...

.github/actions/*/action.yml: Fill in action.yml with every input and output; mark required ones clearly
For composite actions and path context, use "${{ github.action_path }}" when referencing sibling scripts for portability

📄 Source: CodeRabbit Inference Engine (AGENTS.md)

List of files the instruction was applied to:

  • .github/actions/ratchet-coverage/action.yml
🧬 Code Graph Analysis (2)
scripts/ratchet_coverage/ratchet_coverage.py (2)
scripts/ratchet_coverage/install_cargo_llvm_cov.py (1)
  • main (10-11)
scripts/ratchet_coverage/run_coverage.py (1)
  • main (21-32)
scripts/ratchet_coverage/run_coverage.py (2)
scripts/ratchet_coverage/install_cargo_llvm_cov.py (1)
  • main (10-11)
scripts/ratchet_coverage/ratchet_coverage.py (1)
  • main (20-36)
🪛 Pylint (3.3.7)
scripts/ratchet_coverage/install_cargo_llvm_cov.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 6-6: Unable to import 'plumbum.cmd'

(E0401)


[error] 7-7: Unable to import 'typer'

(E0401)


[convention] 10-10: Missing function or method docstring

(C0116)

scripts/ratchet_coverage/ratchet_coverage.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 7-7: Unable to import 'typer'

(E0401)


[convention] 20-20: Missing function or method docstring

(C0116)

scripts/ratchet_coverage/run_coverage.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 9-9: Unable to import 'typer'

(E0401)


[error] 10-10: Unable to import 'plumbum.cmd'

(E0401)


[convention] 13-13: Missing function or method docstring

(C0116)


[convention] 21-21: Missing function or method docstring

(C0116)

🪛 Ruff (0.11.9)
scripts/ratchet_coverage/ratchet_coverage.py

21-23: Do not perform function call typer.Option in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)


22-22: Trailing comma missing

Add trailing comma

(COM812)

scripts/ratchet_coverage/run_coverage.py

23-23: Do not perform function call typer.Option in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

🔇 Additional comments (3)
scripts/ratchet_coverage/run_coverage.py (1)

21-32: Well-structured coverage execution and output handling.

The implementation correctly handles optional arguments, executes the coverage command, and writes the output to the GitHub Actions output file. The use of shlex.split() for argument parsing is appropriate.

scripts/ratchet_coverage/ratchet_coverage.py (2)

10-17: Excellent error handling in baseline reading.

The read_baseline function handles edge cases well, returning 0.0 for missing files or invalid content. This ensures the ratchet system starts with a sensible baseline.


20-36: Robust coverage comparison and baseline management.

The implementation correctly compares current coverage against the baseline, provides clear feedback, and updates the baseline file when coverage doesn't decrease. The directory creation ensures the baseline file can be written successfully.

Comment thread scripts/ratchet_coverage/install_cargo_llvm_cov.py
Comment thread scripts/ratchet_coverage/install_cargo_llvm_cov.py Outdated
${{ runner.os }}-llvmcov-
- name: Install cargo-llvm-cov
run: cargo install cargo-llvm-cov
run: uv run --script scripts/ratchet_coverage/install_cargo_llvm_cov.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Move scripts to action directory for portability.

The coding guidelines specify that actions should reference sibling scripts using ${{ github.action_path }} for portability. The current implementation references scripts in a global directory which violates this guideline.

Consider moving the scripts to .github/actions/ratchet-coverage/src/ and updating the reference:

-      run: uv run --script scripts/ratchet_coverage/install_cargo_llvm_cov.py
+      run: uv run --script ${{ github.action_path }}/src/install_cargo_llvm_cov.py
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
run: uv run --script scripts/ratchet_coverage/install_cargo_llvm_cov.py
run: uv run --script ${{ github.action_path }}/src/install_cargo_llvm_cov.py
🤖 Prompt for AI Agents
In .github/actions/ratchet-coverage/action.yml at line 36, the script path is
referenced globally which breaks portability guidelines. Move the scripts to
.github/actions/ratchet-coverage/src/ and update the run command to use the
relative path with ${{ github.action_path }} to reference the script, ensuring
the action is portable and follows coding standards.


mkdir -p "$(dirname "$file")"
printf '%.2f' "$current" > "$file"
run: uv run --script scripts/ratchet_coverage/ratchet_coverage.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use action path for script reference.

Apply the same portability fix as suggested for the other scripts.

-      run: uv run --script scripts/ratchet_coverage/ratchet_coverage.py
+      run: uv run --script ${{ github.action_path }}/src/ratchet_coverage.py
🤖 Prompt for AI Agents
In .github/actions/ratchet-coverage/action.yml at line 49, the script path is
hardcoded and not using the action's directory path, which reduces portability.
Update the run command to reference the script using the action's path variable
(e.g., ${{ github.action_path }}) to ensure the script path is correctly
resolved regardless of where the action is run.

echo "$output"
percent=$(echo "$output" | grep -oEm1 '[0-9]+(\.[0-9]+)?%' | tr -d '%')
echo "percent=$percent" >> "$GITHUB_OUTPUT"
run: uv run --script scripts/ratchet_coverage/run_coverage.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use action path for script reference.

Apply the same portability fix as suggested for the install script.

-      run: uv run --script scripts/ratchet_coverage/run_coverage.py
+      run: uv run --script ${{ github.action_path }}/src/run_coverage.py
🤖 Prompt for AI Agents
In .github/actions/ratchet-coverage/action.yml at line 46, the script path is
hardcoded and not using the action's directory path, which reduces portability.
Update the run command to reference the script using the action's path variable
(e.g., ${{ github.action_path }}) to ensure the script is correctly located
regardless of where the action is run from.

Comment on lines +13 to +18
def extract_percent(output: str) -> str:
match = re.search(r"([0-9]+(?:\.[0-9]+)?)%", output)
if not match:
typer.echo("Could not parse coverage percent", err=True)
raise typer.Exit(code=1)
return match.group(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider making the regex more specific.

The current regex r"([0-9]+(?:\.[0-9]+)?)%" will match any percentage in the output. Consider making it more specific to avoid false matches, such as looking for coverage-related context.

 def extract_percent(output: str) -> str:
-    match = re.search(r"([0-9]+(?:\.[0-9]+)?)%", output)
+    # Look for coverage percentage in typical llvm-cov output format
+    match = re.search(r"(?:coverage|Coverage).*?([0-9]+(?:\.[0-9]+)?)%", output)
     if not match:
         typer.echo("Could not parse coverage percent", err=True)
         raise typer.Exit(code=1)
     return match.group(1)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def extract_percent(output: str) -> str:
match = re.search(r"([0-9]+(?:\.[0-9]+)?)%", output)
if not match:
typer.echo("Could not parse coverage percent", err=True)
raise typer.Exit(code=1)
return match.group(1)
def extract_percent(output: str) -> str:
# Look for coverage percentage in typical llvm-cov output format
match = re.search(r"(?:coverage|Coverage).*?([0-9]+(?:\.[0-9]+)?)%", output)
if not match:
typer.echo("Could not parse coverage percent", err=True)
raise typer.Exit(code=1)
return match.group(1)
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 13-13: Missing function or method docstring

(C0116)

🤖 Prompt for AI Agents
In scripts/ratchet_coverage/run_coverage.py around lines 13 to 18, the regex
used to extract the coverage percentage is too general and may match unrelated
percentages. Update the regex to include coverage-specific context, such as
preceding keywords like "coverage" or "covered", to ensure it only matches the
intended coverage percentage in the output.

leynos and others added 2 commits July 5, 2025 20:51
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@leynos leynos merged commit ca498c5 into main Jul 5, 2025
1 check was pending
@leynos leynos deleted the codex/extract-and-convert-bash-scripts-to-python branch July 5, 2025 19:53
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
scripts/ratchet_coverage/run_coverage.py (1)

13-18: Consider making the regex more specific.

The current regex r"([0-9]+(?:\.[0-9]+)?)%" will match any percentage in the output. Consider making it more specific to avoid false matches, such as looking for coverage-related context.

 def extract_percent(output: str) -> str:
-    match = re.search(r"([0-9]+(?:\.[0-9]+)?)%", output)
+    # Look for coverage percentage in typical llvm-cov output format
+    match = re.search(r"(?:coverage|Coverage).*?([0-9]+(?:\.[0-9]+)?)%", output)
     if not match:
         typer.echo("Could not parse coverage percent", err=True)
         raise typer.Exit(code=1)
-    return match[1]
+    return match[1]
scripts/ratchet_coverage/install_cargo_llvm_cov.py (1)

19-20: Consider simplifying the entry point.

Since this script doesn't require CLI arguments, the use of typer.run() is unnecessary. A simple function call would suffice.

 if __name__ == "__main__":
-    typer.run(main)
+    main()
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 149aa78 and f19ff4e.

📒 Files selected for processing (2)
  • scripts/ratchet_coverage/install_cargo_llvm_cov.py (1 hunks)
  • scripts/ratchet_coverage/run_coverage.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/ratchet_coverage/install_cargo_llvm_cov.py (2)
scripts/ratchet_coverage/run_coverage.py (1)
  • main (21-32)
scripts/ratchet_coverage/ratchet_coverage.py (1)
  • main (20-36)
🪛 Ruff (0.11.9)
scripts/ratchet_coverage/install_cargo_llvm_cov.py

14-14: Do not catch blind exception: Exception

(BLE001)


16-16: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

scripts/ratchet_coverage/run_coverage.py

23-23: Do not perform function call typer.Option in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

🪛 Pylint (3.3.7)
scripts/ratchet_coverage/install_cargo_llvm_cov.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 6-6: Unable to import 'plumbum.cmd'

(E0401)


[error] 7-7: Unable to import 'typer'

(E0401)


[convention] 10-10: Missing function or method docstring

(C0116)

scripts/ratchet_coverage/run_coverage.py

[convention] 1-1: Missing module docstring

(C0114)


[error] 9-9: Unable to import 'typer'

(E0401)


[error] 10-10: Unable to import 'plumbum.cmd'

(E0401)


[convention] 13-13: Missing function or method docstring

(C0116)


[convention] 21-21: Missing function or method docstring

(C0116)

🔇 Additional comments (4)
scripts/ratchet_coverage/run_coverage.py (2)

1-11: LGTM!

The script header and imports are well-structured and appropriate for a uv script.


35-36: LGTM!

Standard typer application entry point.

scripts/ratchet_coverage/install_cargo_llvm_cov.py (2)

1-5: LGTM! Modern script setup with appropriate dependencies.

The use of uv run --script with inline dependency specification is a clean approach for executable Python scripts.


6-7: LGTM! Appropriate imports for the script's functionality.

The imports are well-chosen for command execution and CLI interface.

Comment on lines +21 to +32
def main(
args: str = typer.Option("", envvar="INPUT_ARGS"),
github_output: Path = typer.Option(..., envvar="GITHUB_OUTPUT"),
) -> None:
cmd = cargo["llvm-cov", "--summary-only"]
if args:
cmd = cmd[shlex.split(args)]
output = cmd()
typer.echo(output)
percent = extract_percent(output)
with github_output.open("a") as fh:
fh.write(f"percent={percent}\n")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Address the function call in argument defaults.

The use of typer.Option(...) in the function signature triggers a static analysis warning (Ruff B008). While this works with typer, consider refactoring to follow best practices.

 def main(
-    args: str = typer.Option("", envvar="INPUT_ARGS"),
-    github_output: Path = typer.Option(..., envvar="GITHUB_OUTPUT"),
+    args: str = typer.Option("", envvar="INPUT_ARGS"),
+    github_output: Path = typer.Option(..., envvar="GITHUB_OUTPUT"),
 ) -> None:
+    """Run cargo llvm-cov and extract coverage percentage."""
     cmd = cargo["llvm-cov", "--summary-only"]
     if args:
         cmd = cmd[shlex.split(args)]
     output = cmd()
     typer.echo(output)
     percent = extract_percent(output)
     with github_output.open("a") as fh:
         fh.write(f"percent={percent}\n")

Alternative approach to avoid the warning:

+def main(
+    args: str = "",
+    github_output: str = "",
+) -> None:
+    """Run cargo llvm-cov and extract coverage percentage."""
+    # Get values from environment if not provided
+    if not args:
+        args = os.environ.get("INPUT_ARGS", "")
+    if not github_output:
+        github_output = os.environ["GITHUB_OUTPUT"]
+    
+    github_output_path = Path(github_output)
+    # ... rest of the function
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def main(
args: str = typer.Option("", envvar="INPUT_ARGS"),
github_output: Path = typer.Option(..., envvar="GITHUB_OUTPUT"),
) -> None:
cmd = cargo["llvm-cov", "--summary-only"]
if args:
cmd = cmd[shlex.split(args)]
output = cmd()
typer.echo(output)
percent = extract_percent(output)
with github_output.open("a") as fh:
fh.write(f"percent={percent}\n")
def main(
args: str = typer.Option("", envvar="INPUT_ARGS"),
github_output: Path = typer.Option(..., envvar="GITHUB_OUTPUT"),
) -> None:
"""Run cargo llvm-cov and extract coverage percentage."""
cmd = cargo["llvm-cov", "--summary-only"]
if args:
cmd = cmd[shlex.split(args)]
output = cmd()
typer.echo(output)
percent = extract_percent(output)
with github_output.open("a") as fh:
fh.write(f"percent={percent}\n")
🧰 Tools
🪛 Ruff (0.11.9)

23-23: Do not perform function call typer.Option in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

🪛 Pylint (3.3.7)

[convention] 21-21: Missing function or method docstring

(C0116)

🤖 Prompt for AI Agents
In scripts/ratchet_coverage/run_coverage.py around lines 21 to 32, the use of
typer.Option(...) directly in the function argument defaults causes a static
analysis warning (Ruff B008). To fix this, remove the typer.Option calls from
the function signature and instead define the options inside the function body
or use a separate function to parse the options. This avoids calling functions
in default argument values and aligns with best practices.

Comment on lines +10 to +16
def main() -> None:
try:
cargo["install", "cargo-llvm-cov"]()
typer.echo("cargo-llvm-cov installed successfully")
except Exception as e:
typer.echo(f"Failed to install cargo-llvm-cov: {e}", err=True)
raise typer.Exit(code=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve exception handling specificity and add documentation.

Good work adding error handling as suggested in the previous review. Consider these improvements:

  1. Add function docstring for clarity
  2. Use more specific exception handling - catch plumbum.ProcessExecutionError instead of broad Exception
  3. Improve exception chaining as flagged by static analysis
 def main() -> None:
+    """Install cargo-llvm-cov using cargo."""
     try:
         cargo["install", "cargo-llvm-cov"]()
         typer.echo("cargo-llvm-cov installed successfully")
-    except Exception as e:
+    except Exception as e:
         typer.echo(f"Failed to install cargo-llvm-cov: {e}", err=True)
-        raise typer.Exit(code=1)
+        raise typer.Exit(code=1) from e

Note: For more specific exception handling, you could import from plumbum import ProcessExecutionError and catch that instead of Exception.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def main() -> None:
try:
cargo["install", "cargo-llvm-cov"]()
typer.echo("cargo-llvm-cov installed successfully")
except Exception as e:
typer.echo(f"Failed to install cargo-llvm-cov: {e}", err=True)
raise typer.Exit(code=1)
def main() -> None:
"""Install cargo-llvm-cov using cargo."""
try:
cargo["install", "cargo-llvm-cov"]()
typer.echo("cargo-llvm-cov installed successfully")
except Exception as e:
typer.echo(f"Failed to install cargo-llvm-cov: {e}", err=True)
raise typer.Exit(code=1) from e
🧰 Tools
🪛 Ruff (0.11.9)

14-14: Do not catch blind exception: Exception

(BLE001)


16-16: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🪛 Pylint (3.3.7)

[convention] 10-10: Missing function or method docstring

(C0116)

🤖 Prompt for AI Agents
In scripts/ratchet_coverage/install_cargo_llvm_cov.py around lines 10 to 16,
improve the main function by adding a clear docstring explaining its purpose.
Replace the broad Exception catch with a more specific catch for
plumbum.ProcessExecutionError by importing it from plumbum. Also, when
re-raising the exception, use exception chaining with 'raise ... from e' to
preserve the original traceback for better debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant