Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 5 additions & 33 deletions .github/actions/ratchet-coverage/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ runs:
restore-keys: |
${{ 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.

shell: bash
- if: runner.os == 'Windows'
name: Install bc (MSYS2)
Expand All @@ -43,40 +43,12 @@ runs:
path-type: inherit
- name: Run coverage
id: cov
run: |
set -euo pipefail
output=$(cargo llvm-cov --summary-only ${{ inputs.args }})
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.

shell: bash
- name: Ratchet coverage
run: |
set -euo pipefail
file="${{ inputs.baseline-file }}"
current="${{ steps.cov.outputs.percent }}"
baseline=0
if [ -f "$file" ]; then
baseline=$(cat "$file")
fi
echo "Current coverage: $current%"
echo "Baseline coverage: $baseline%"

if ! echo "$current" | grep -Eq '^[0-9]+(\.[0-9]+)?$'; then
echo "Invalid coverage value: $current" >&2
exit 1
fi
if ! echo "$baseline" | grep -Eq '^[0-9]+(\.[0-9]+)?$'; then
baseline=0
fi

if (( $(bc -l <<<"$current < $baseline") )); then
echo "Coverage decreased" >&2
exit 1
fi

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.

env:
CURRENT_PERCENT: ${{ steps.cov.outputs.percent }}
shell: bash
- name: Save baseline
if: success()
Expand Down
20 changes: 20 additions & 0 deletions scripts/ratchet_coverage/install_cargo_llvm_cov.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env -S uv run --script
# /// script
# requires-python = ">=3.12"
# dependencies = ["plumbum", "typer"]
# ///
from plumbum.cmd import cargo
import typer


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)
Comment on lines +10 to +16
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.



if __name__ == "__main__":
typer.run(main)
Comment thread
leynos marked this conversation as resolved.
40 changes: 40 additions & 0 deletions scripts/ratchet_coverage/ratchet_coverage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#!/usr/bin/env -S uv run --script
# /// script
# requires-python = ">=3.12"
# dependencies = ["plumbum", "typer"]
# ///
from pathlib import Path
import typer


def read_baseline(file: Path) -> float:
"""Return the stored baseline coverage or 0.0 if missing/invalid."""
if file.is_file():
try:
return float(file.read_text().strip())
except ValueError:
return 0.0
return 0.0


def main(
baseline_file: Path = typer.Option(
Path(".coverage-baseline"), envvar="INPUT_BASELINE_FILE"
),
current: float = typer.Option(..., envvar="CURRENT_PERCENT"),
) -> None:
baseline = read_baseline(baseline_file)

typer.echo(f"Current coverage: {current}%")
typer.echo(f"Baseline coverage: {baseline}%")

if current < baseline:
typer.echo("Coverage decreased", err=True)
raise typer.Exit(code=1)

baseline_file.parent.mkdir(parents=True, exist_ok=True)
baseline_file.write_text(f"{current:.2f}")


if __name__ == "__main__":
typer.run(main)
36 changes: 36 additions & 0 deletions scripts/ratchet_coverage/run_coverage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/usr/bin/env -S uv run --script
# /// script
# requires-python = ">=3.12"
# dependencies = ["plumbum", "typer"]
# ///
from pathlib import Path
import re
import shlex
import typer
from plumbum.cmd import cargo


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[1]


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")
Comment on lines +21 to +32
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.



if __name__ == "__main__":
typer.run(main)