Convert generate-coverage to Python scripts#23
Conversation
Reviewer's GuideReplaces inline bash in the generate-coverage composite action with uv-run Python helper scripts, refactors detection and command-generation logic into modular functions with match/case and docstrings, adds documentation for the Python scripting approach, and applies ruff formatting to the new scripts. Class diagram for new generate_coverage Python helpersclassDiagram
class CoverageFmt {
+LCOV
+COBERTURA
+COVERAGEPY
}
class CoverageFmtParam {
+name
+convert(value, param, ctx)
}
class detect_py {
+get_lang() str
+main(fmt, github_output)
}
class run_python_py {
+coverage_cmd_for_fmt(fmt, out)
+main(output_path, lang, fmt, github_output)
}
class run_rust_py {
+get_cargo_coverage_cmd(fmt, out, features, with_default)
+main(output_path, features, with_default, lang, fmt, github_output)
}
class merge_cobertura_py {
+main(rust_file, python_file, output_path)
}
class set_outputs_py {
+main(output_path, fmt, github_output)
}
CoverageFmtParam --> CoverageFmt
detect_py --> CoverageFmt
detect_py --> CoverageFmtParam
run_python_py --> detect_py
run_rust_py --> detect_py
merge_cobertura_py --> run_rust_py
merge_cobertura_py --> run_python_py
set_outputs_py --> detect_py
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 40 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe composite GitHub Action for coverage generation has been refactored to replace inline shell scripting with modular Python scripts. These scripts handle language and format detection, run coverage for Rust and Python, merge Cobertura reports for mixed projects, and set outputs, all orchestrated via Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Action
participant detect.py
participant run_rust.py
participant run_python.py
participant merge_cobertura.py
participant set_outputs.py
GitHub Action->>detect.py: Detect language & format
detect.py-->>GitHub Action: Outputs language & format
alt Rust only
GitHub Action->>run_rust.py: Run Rust coverage
run_rust.py-->>GitHub Action: Outputs coverage path
else Python only
GitHub Action->>run_python.py: Run Python coverage
run_python.py-->>GitHub Action: Outputs coverage path
else Mixed (Rust & Python)
GitHub Action->>run_rust.py: Run Rust coverage
GitHub Action->>run_python.py: Run Python coverage
GitHub Action->>merge_cobertura.py: Merge Rust & Python reports
merge_cobertura.py-->>GitHub Action: Outputs merged path
end
GitHub Action->>set_outputs.py: Set outputs for downstream steps
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/generate_coverage/run_python.py:48` </location>
<code_context>
+ except ProcessExecutionError as exc:
+ raise typer.Exit(code=exc.retcode or 1) from exc
+
+ if fmt == "coveragepy":
+ Path(".coverage").rename(out)
+
+ with github_output.open("a") as fh:
</code_context>
<issue_to_address>
Potential file overwrite if .coverage already exists at output location.
Path.rename() will fail with FileExistsError if the target file exists. Use Path.replace() or remove the existing file first to avoid unexpected errors.
</issue_to_address>
### Comment 2
<location> `scripts/generate_coverage/merge_cobertura.py:14` </location>
<code_context>
+from plumbum.commands.processes import ProcessExecutionError
+
+
+class ExistingFile(click.ParamType):
+ name = "file"
+
</code_context>
<issue_to_address>
Consider replacing the custom ExistingFile class with Typer's built-in file validation options for input arguments.
```suggestion
@@
-import click
from pathlib import Path
-import typer
+import typer
-class ExistingFile(click.ParamType):
- name = "file"
-
- def __init__(self, kind: str) -> None:
- """Create a validator that ensures the file exists."""
- self.kind = kind
-
- def convert(
- self, value: str, param: click.Parameter | None, ctx: click.Context | None
- ) -> Path: # type: ignore[override]
- """Validate that the provided path points to an existing file."""
- path = Path(value)
- if not path.is_file():
- self.fail(f"{self.kind} file not found: {value}", param, ctx)
- return path
@@
-def main(
- rust_file: Path = typer.Option(
- ..., envvar="RUST_FILE", type=ExistingFile("Rust coverage")
- ),
- python_file: Path = typer.Option(
- ..., envvar="PYTHON_FILE", type=ExistingFile("Python coverage")
- ),
- output_path: Path = typer.Option(..., envvar="OUTPUT_PATH"),
-) -> None:
+def main(
+ rust_file: Path = typer.Option(
+ ..., envvar="RUST_FILE", exists=True, file_okay=True, dir_okay=False
+ ),
+ python_file: Path = typer.Option(
+ ..., envvar="PYTHON_FILE", exists=True, file_okay=True, dir_okay=False
+ ),
+ output_path: Path = typer.Option(..., envvar="OUTPUT_PATH"),
+) -> None:
```
Steps:
1. Remove the `click` import and `ExistingFile` class entirely.
2. For each file argument, use Typer’s built-in `exists`, `file_okay`, and `dir_okay` options.
3. Keep `Path` as the parameter type so Typer still returns a `Path` instance.
4. No change to downstream logic—all functionality is preserved.
</issue_to_address>
### Comment 3
<location> `docs/python-action-scripts.md:18` </location>
<code_context>
+
+The [Typer](https://typer.tiangolo.com/) library is used for argument parsing and
+error handling, while [plumbum](https://plumbum.readthedocs.io/) provides simple
+command execution. By isolating logic in Python we keep the composite action
+YAML minimal and benefit from better readability and testability.
--- /dev/null
</code_context>
<issue_to_address>
The sentence uses "we", which should be avoided per the instructions.
Please rephrase to avoid "we". For example: "By isolating logic in Python, the composite action YAML remains minimal and benefits from better readability and testability."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if fmt == "coveragepy": | ||
| Path(".coverage").rename(out) |
There was a problem hiding this comment.
issue (bug_risk): Potential file overwrite if .coverage already exists at output location.
Path.rename() will fail with FileExistsError if the target file exists. Use Path.replace() or remove the existing file first to avoid unexpected errors.
| from plumbum.commands.processes import ProcessExecutionError | ||
|
|
||
|
|
||
| class ExistingFile(click.ParamType): |
There was a problem hiding this comment.
issue (complexity): Consider replacing the custom ExistingFile class with Typer's built-in file validation options for input arguments.
| class ExistingFile(click.ParamType): | |
| @@ | |
| -import click | |
| from pathlib import Path | |
| -import typer | |
| +import typer | |
| -class ExistingFile(click.ParamType): | |
| - name = "file" | |
| - | |
| - def __init__(self, kind: str) -> None: | |
| - """Create a validator that ensures the file exists.""" | |
| - self.kind = kind | |
| - | |
| - def convert( | |
| - self, value: str, param: click.Parameter | None, ctx: click.Context | None | |
| - ) -> Path: # type: ignore[override] | |
| - """Validate that the provided path points to an existing file.""" | |
| - path = Path(value) | |
| - if not path.is_file(): | |
| - self.fail(f"{self.kind} file not found: {value}", param, ctx) | |
| - return path | |
| @@ | |
| -def main( | |
| - rust_file: Path = typer.Option( | |
| - ..., envvar="RUST_FILE", type=ExistingFile("Rust coverage") | |
| - ), | |
| - python_file: Path = typer.Option( | |
| - ..., envvar="PYTHON_FILE", type=ExistingFile("Python coverage") | |
| - ), | |
| - output_path: Path = typer.Option(..., envvar="OUTPUT_PATH"), | |
| -) -> None: | |
| +def main( | |
| + rust_file: Path = typer.Option( | |
| + ..., envvar="RUST_FILE", exists=True, file_okay=True, dir_okay=False | |
| + ), | |
| + python_file: Path = typer.Option( | |
| + ..., envvar="PYTHON_FILE", exists=True, file_okay=True, dir_okay=False | |
| + ), | |
| + output_path: Path = typer.Option(..., envvar="OUTPUT_PATH"), | |
| +) -> None: |
Steps:
- Remove the
clickimport andExistingFileclass entirely. - For each file argument, use Typer’s built-in
exists,file_okay, anddir_okayoptions. - Keep
Pathas the parameter type so Typer still returns aPathinstance. - No change to downstream logic—all functionality is preserved.
|
|
||
| The [Typer](https://typer.tiangolo.com/) library is used for argument parsing and | ||
| error handling, while [plumbum](https://plumbum.readthedocs.io/) provides simple | ||
| command execution. By isolating logic in Python we keep the composite action |
There was a problem hiding this comment.
suggestion (review_instructions): The sentence uses "we", which should be avoided per the instructions.
Please rephrase to avoid "we". For example: "By isolating logic in Python, the composite action YAML remains minimal and benefits from better readability and testability."
Review instructions:
Path patterns: **/*.md
Instructions:
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
| cargo = Path("Cargo.toml").is_file() | ||
| python = Path("pyproject.toml").is_file() | ||
| if cargo and python: | ||
| return "mixed" | ||
| if cargo: | ||
| return "rust" | ||
| if python: | ||
| return "python" | ||
| typer.echo("Neither Cargo.toml nor pyproject.toml found", err=True) | ||
| raise typer.Exit(code=1) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Hoist a repeated condition into a parent condition (
hoist-repeated-if-condition) - Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.github/actions/generate-coverage/action.yml(2 hunks)docs/python-action-scripts.md(1 hunks)scripts/generate_coverage/detect.py(1 hunks)scripts/generate_coverage/merge_cobertura.py(1 hunks)scripts/generate_coverage/run_python.py(1 hunks)scripts/generate_coverage/run_rust.py(1 hunks)scripts/generate_coverage/set_outputs.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/python-action-scripts.md
`.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/generate-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/generate-coverage/action.yml
🧬 Code Graph Analysis (2)
scripts/generate_coverage/set_outputs.py (3)
scripts/generate_coverage/run_python.py (1)
main(30-52)scripts/generate_coverage/detect.py (1)
main(46-68)scripts/generate_coverage/run_rust.py (1)
main(26-51)
scripts/generate_coverage/run_python.py (3)
scripts/generate_coverage/detect.py (1)
main(46-68)scripts/generate_coverage/set_outputs.py (1)
main(11-19)scripts/generate_coverage/run_rust.py (1)
main(26-51)
🪛 Ruff (0.11.9)
scripts/generate_coverage/set_outputs.py
1-1: Shebang is present but file is not executable
(EXE001)
12-12: 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)
14-14: 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)
scripts/generate_coverage/run_rust.py
1-1: Shebang is present but file is not executable
(EXE001)
14-14: Boolean-typed positional argument in function definition
(FBT001)
14-14: Trailing comma missing
Add trailing comma
(COM812)
27-27: 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)
29-29: Boolean-typed positional argument in function definition
(FBT001)
29-29: Boolean positional value in function call
(FBT003)
32-32: 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)
46-46: Trailing comma missing
Add trailing comma
(COM812)
scripts/generate_coverage/run_python.py
1-1: Shebang is present but file is not executable
(EXE001)
14-14: Missing return type annotation for public function coverage_cmd_for_fmt
(ANN201)
31-31: 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)
34-34: 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)
scripts/generate_coverage/merge_cobertura.py
1-1: Shebang is present but file is not executable
(EXE001)
22-22: Trailing comma missing
Add trailing comma
(COM812)
32-34: 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)
33-33: Do not perform function call ExistingFile in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-33: Trailing comma missing
Add trailing comma
(COM812)
35-37: 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)
36-36: Do not perform function call ExistingFile in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Trailing comma missing
Add trailing comma
(COM812)
38-38: 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)
45-45: Trailing comma missing
Add trailing comma
(COM812)
scripts/generate_coverage/detect.py
1-1: Shebang is present but file is not executable
(EXE001)
23-23: Trailing comma missing
Add trailing comma
(COM812)
47-49: 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)
48-48: Do not perform function call CoverageFmtParam in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
48-48: Trailing comma missing
Add trailing comma
(COM812)
50-50: 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/generate_coverage/set_outputs.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 8-8: Unable to import 'typer'
(E0401)
scripts/generate_coverage/run_rust.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 8-8: Unable to import 'typer'
(E0401)
[error] 9-9: Unable to import 'plumbum.cmd'
(E0401)
[error] 10-10: Unable to import 'plumbum.commands.processes'
(E0401)
[refactor] 26-26: Too many arguments (6/5)
(R0913)
[refactor] 26-26: Too many positional arguments (6/5)
(R0917)
scripts/generate_coverage/run_python.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 8-8: Unable to import 'typer'
(E0401)
[error] 9-9: Unable to import 'plumbum'
(E0401)
[error] 10-10: Unable to import 'plumbum.cmd'
(E0401)
[error] 11-11: Unable to import 'plumbum.commands.processes'
(E0401)
scripts/generate_coverage/merge_cobertura.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)
[error] 11-11: Unable to import 'plumbum.commands.processes'
(E0401)
[convention] 14-14: Missing class docstring
(C0115)
scripts/generate_coverage/detect.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'typer'
(E0401)
[convention] 13-13: Missing class docstring
(C0115)
[convention] 19-19: Missing class docstring
(C0115)
[refactor] 22-22: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🪛 LanguageTool
docs/python-action-scripts.md
[style] ~5-~5: Since ownership is already implied, this phrasing may be redundant.
Context: ...uv run --script`. Each script declares its own dependencies via the special comment he...
(PRP_OWN)
[uncategorized] ~18-~18: Possible missing comma found.
Context: ...mmand execution. By isolating logic in Python we keep the composite action YAML minim...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (14)
scripts/generate_coverage/set_outputs.py (2)
11-24: LGTM! Clean and focused implementation.The script correctly implements the output setting logic with proper error handling and follows the established pattern from other scripts in the same directory.
6-9: Consider adding a module docstring.Adding a module docstring would improve code documentation and address the static analysis hint.
from pathlib import Path +"""Set GitHub Actions output variables for coverage workflow.""" + import typerLikely an incorrect or invalid review comment.
docs/python-action-scripts.md (1)
1-20: Excellent documentation for the Python script approach.The documentation clearly explains the rationale and implementation details for using Python helper scripts with uv. The examples are helpful and the explanation of benefits is concise.
scripts/generate_coverage/merge_cobertura.py (2)
14-29: Well-implemented file validation class.The
ExistingFileclass provides good validation with clear error messages. The implementation correctly extends click.ParamType and provides meaningful feedback.
31-51: Excellent error handling and cleanup logic.The script properly handles command execution errors, provides meaningful error messages, and cleans up input files after successful merging. The implementation follows the established pattern from other scripts.
.github/actions/generate-coverage/action.yml (3)
35-43: Excellent refactoring to use Python scripts.The replacement of inline shell commands with dedicated Python scripts improves maintainability and testability. The environment variables properly pass data between steps.
63-76: Well-structured conditional execution.The conditional logic for mixed projects is preserved whilst using the new Python scripts. The environment variable passing ensures proper data flow between the Rust and Python coverage steps and the merge operation.
77-81: Clean output setting with environment variables.The final output setting step correctly uses environment variables to pass the detected format, maintaining consistency with the overall refactoring approach.
scripts/generate_coverage/run_python.py (1)
30-53: Excellent implementation with proper error handling.The script correctly handles different coverage formats, adjusts output paths for mixed projects, ensures output directories exist, and provides appropriate error handling. The logic for renaming the
.coveragefile for the coveragepy format is particularly well-implemented.scripts/generate_coverage/run_rust.py (2)
36-38: LGTM! Excellent handling of mixed language projects.The logic to append
.rustto the output filename for mixed projects is well-implemented and ensures proper separation of coverage files.
50-51: LGTM! Proper GitHub Actions integration.The output writing to
GITHUB_OUTPUTfollows the correct format for GitHub Actions.scripts/generate_coverage/detect.py (3)
32-44: LGTM! Excellent language detection logic.The function correctly identifies project types based on the presence of
Cargo.tomlandpyproject.tomlfiles. The logic handles all possible combinations appropriately.
55-65: Brilliant use of match/case for validation.The validation logic using match/case statements is clean and comprehensive. It properly handles all incompatible format/language combinations:
- Rust projects can't use coveragepy format
- Python projects can't use lcov format
- Mixed projects only support cobertura format
66-68: LGTM! Proper GitHub Actions output format.The output writing correctly follows GitHub Actions conventions by appending to the output file.
| @@ -0,0 +1,23 @@ | |||
| #!/usr/bin/env -S uv run --script | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make the script executable.
The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/set_outputs.py🧰 Tools
🪛 Ruff (0.11.9)
1-1: Shebang is present but file is not executable
(EXE001)
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
🤖 Prompt for AI Agents
In scripts/generate_coverage/set_outputs.py at line 1, the script has a shebang
indicating it should be executable, but the file currently lacks executable
permissions. Fix this by running the command chmod +x
scripts/generate_coverage/set_outputs.py to add executable permissions to the
file.
| Some actions use short helper scripts for logic that would otherwise live in bash. | ||
| These scripts are stored under `scripts/` and executed with [`uv`](https://github.com/astral-sh/uv) using | ||
| `uv run --script`. Each script declares its own dependencies via the | ||
| special comment header understood by uv. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing redundant phrasing.
The phrase "its own dependencies" is redundant since ownership is already implied by context.
-Each script declares its own dependencies via the
+Each script declares dependencies via the📝 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.
| Some actions use short helper scripts for logic that would otherwise live in bash. | |
| These scripts are stored under `scripts/` and executed with [`uv`](https://github.com/astral-sh/uv) using | |
| `uv run --script`. Each script declares its own dependencies via the | |
| special comment header understood by uv. | |
| Some actions use short helper scripts for logic that would otherwise live in bash. | |
| These scripts are stored under `scripts/` and executed with [`uv`](https://github.com/astral-sh/uv) using | |
| `uv run --script`. Each script declares dependencies via the | |
| special comment header understood by uv. |
🧰 Tools
🪛 LanguageTool
[style] ~5-~5: Since ownership is already implied, this phrasing may be redundant.
Context: ...uv run --script`. Each script declares its own dependencies via the special comment he...
(PRP_OWN)
🤖 Prompt for AI Agents
In docs/python-action-scripts.md around lines 3 to 6, remove the phrase "its
own" before "dependencies" to eliminate redundancy, so the sentence simply
states that each script declares dependencies via the special comment header
understood by uv.
| @@ -0,0 +1,54 @@ | |||
| #!/usr/bin/env -S uv run --script | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make the script executable.
The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/merge_cobertura.py🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
🪛 Ruff (0.11.9)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In scripts/generate_coverage/merge_cobertura.py at line 1, the script has a
shebang indicating it should be executable, but the file currently lacks
executable permissions. Fix this by running the command chmod +x
scripts/generate_coverage/merge_cobertura.py to add executable permissions to
the file.
| def get_cargo_coverage_cmd( | ||
| fmt: str, out: Path, features: str, with_default: bool | ||
| ) -> list[str]: | ||
| """Return the cargo llvm-cov command arguments.""" | ||
| args = ["llvm-cov", "--workspace"] | ||
| if not with_default: | ||
| args.append("--no-default-features") | ||
| if features: | ||
| args += ["--features", features] | ||
| args += [f"--{fmt}", "--output-path", str(out)] | ||
| return args | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider refactoring boolean parameter and add trailing comma.
The function has a boolean positional parameter which can be confusing. Consider making it keyword-only or using an enum for clarity.
def get_cargo_coverage_cmd(
- fmt: str, out: Path, features: str, with_default: bool
+ fmt: str, out: Path, features: str, *, with_default: bool,
) -> list[str]:This makes the boolean parameter keyword-only, improving readability at call sites.
📝 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.
| def get_cargo_coverage_cmd( | |
| fmt: str, out: Path, features: str, with_default: bool | |
| ) -> list[str]: | |
| """Return the cargo llvm-cov command arguments.""" | |
| args = ["llvm-cov", "--workspace"] | |
| if not with_default: | |
| args.append("--no-default-features") | |
| if features: | |
| args += ["--features", features] | |
| args += [f"--{fmt}", "--output-path", str(out)] | |
| return args | |
| def get_cargo_coverage_cmd( | |
| fmt: str, out: Path, features: str, *, with_default: bool, | |
| ) -> list[str]: | |
| """Return the cargo llvm-cov command arguments.""" | |
| args = ["llvm-cov", "--workspace"] | |
| if not with_default: | |
| args.append("--no-default-features") | |
| if features: | |
| args += ["--features", features] | |
| args += [f"--{fmt}", "--output-path", str(out)] | |
| return args |
🧰 Tools
🪛 Ruff (0.11.9)
14-14: Boolean-typed positional argument in function definition
(FBT001)
14-14: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In scripts/generate_coverage/run_rust.py around lines 13 to 24, the function
get_cargo_coverage_cmd uses a boolean positional parameter with_default, which
can be unclear at call sites. Refactor the function to make with_default a
keyword-only parameter by adding a * before it in the parameter list, so callers
must specify it by name. Also, add a trailing comma after the last parameter for
cleaner diffs and consistency.
| #!/usr/bin/env -S uv run --script | ||
| # /// script | ||
| # requires-python = ">=3.12" | ||
| # dependencies = ["typer"] | ||
| # /// |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add module docstring and address shebang permissions.
The script is missing a module docstring and the shebang indicates it should be executable.
#!/usr/bin/env -S uv run --script
+"""Detect project language and validate coverage format compatibility."""
# /// script
# requires-python = ">=3.12"
# dependencies = ["typer"]
# ///Also ensure the script has executable permissions:
chmod +x scripts/generate_coverage/detect.py🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
🪛 Ruff (0.11.9)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In scripts/generate_coverage/detect.py at the top of the file (lines 1 to 5),
add a module-level docstring describing the purpose of the script. Also, ensure
the shebang line is correct for execution and set the file's executable
permissions by running chmod +x on the script file.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Make sure each uv-run script step in your composite action actually exports the required INPUT_* env vars (e.g. INPUT_OUTPUT_PATH, INPUT_FEATURES, INPUT_WITH_DEFAULT_FEATURES) so the scripts don’t error on missing required options.
- detect.py mixes click.ParamType with Typer; consider using a Typer-native ParamType (or centralizing your enum) to avoid pulling in click directly and keep consistency across scripts.
- You’ve defined coverage formats as an enum in detect.py but then use raw strings in run_*.py—extract format definitions into a shared module or reuse the enum to prevent drift when formats change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Make sure each uv-run script step in your composite action actually exports the required INPUT_* env vars (e.g. INPUT_OUTPUT_PATH, INPUT_FEATURES, INPUT_WITH_DEFAULT_FEATURES) so the scripts don’t error on missing required options.
- detect.py mixes click.ParamType with Typer; consider using a Typer-native ParamType (or centralizing your enum) to avoid pulling in click directly and keep consistency across scripts.
- You’ve defined coverage formats as an enum in detect.py but then use raw strings in run_*.py—extract format definitions into a shared module or reuse the enum to prevent drift when formats change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
scripts/generate_coverage/set_outputs.py (1)
1-1: Make the script executable.The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/set_outputs.pyscripts/generate_coverage/merge_cobertura.py (1)
1-1: Make the script executable.The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/merge_cobertura.pyscripts/generate_coverage/run_python.py (1)
1-1: Make the script executable.The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/run_python.pyscripts/generate_coverage/run_rust.py (3)
1-1: Make the script executable.The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/run_rust.py
22-24: Address boolean parameter and trailing comma issues.The function has a boolean positional parameter which can be confusing at call sites, and is missing a trailing comma.
def get_cargo_coverage_cmd( - fmt: str, out: Path, features: str, with_default: bool + fmt: str, out: Path, features: str, *, with_default: bool, ) -> list[str]:Making
with_defaultkeyword-only improves readability at call sites.
54-56: Add trailing comma for consistency.Consider adding a trailing comma for better style consistency.
typer.echo( - f"cargo llvm-cov failed with code {exc.retcode}: {exc.stderr}", err=True + f"cargo llvm-cov failed with code {exc.retcode}: {exc.stderr}", err=True, )scripts/generate_coverage/detect.py (1)
1-1: Make the script executable.The shebang indicates this script should be executable, but the file lacks executable permissions.
chmod +x scripts/generate_coverage/detect.py
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
scripts/generate_coverage/detect.py(1 hunks)scripts/generate_coverage/merge_cobertura.py(1 hunks)scripts/generate_coverage/run_python.py(1 hunks)scripts/generate_coverage/run_rust.py(1 hunks)scripts/generate_coverage/set_outputs.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
scripts/generate_coverage/set_outputs.py (4)
scripts/generate_coverage/merge_cobertura.py (1)
main(23-38)scripts/generate_coverage/detect.py (1)
main(63-83)scripts/generate_coverage/run_python.py (1)
main(38-60)scripts/generate_coverage/run_rust.py (1)
main(35-60)
scripts/generate_coverage/detect.py (4)
scripts/generate_coverage/merge_cobertura.py (1)
main(23-38)scripts/generate_coverage/run_python.py (1)
main(38-60)scripts/generate_coverage/run_rust.py (1)
main(35-60)scripts/generate_coverage/set_outputs.py (1)
main(17-25)
scripts/generate_coverage/run_rust.py (4)
scripts/generate_coverage/merge_cobertura.py (1)
main(23-38)scripts/generate_coverage/detect.py (1)
main(63-83)scripts/generate_coverage/run_python.py (1)
main(38-60)scripts/generate_coverage/set_outputs.py (1)
main(17-25)
scripts/generate_coverage/merge_cobertura.py (3)
scripts/generate_coverage/detect.py (1)
main(63-83)scripts/generate_coverage/run_python.py (1)
main(38-60)scripts/generate_coverage/run_rust.py (1)
main(35-60)
🪛 Pylint (3.3.7)
scripts/generate_coverage/set_outputs.py
[error] 10-10: Unable to import 'typer'
(E0401)
scripts/generate_coverage/detect.py
[error] 12-12: Unable to import 'typer'
(E0401)
[refactor] 28-28: Either all return statements in a function should return an expression, or none of them should.
(R1710)
scripts/generate_coverage/run_python.py
[error] 10-10: Unable to import 'typer'
(E0401)
[error] 11-11: Unable to import 'plumbum'
(E0401)
[error] 12-12: Unable to import 'plumbum.cmd'
(E0401)
[error] 13-13: Unable to import 'plumbum.commands.base'
(E0401)
[error] 14-14: Unable to import 'plumbum.commands.processes'
(E0401)
scripts/generate_coverage/run_rust.py
[error] 10-10: Unable to import 'typer'
(E0401)
[error] 11-11: Unable to import 'plumbum.cmd'
(E0401)
[error] 12-12: Unable to import 'plumbum.commands.processes'
(E0401)
[refactor] 35-35: Too many arguments (6/5)
(R0913)
[refactor] 35-35: Too many positional arguments (6/5)
(R0917)
scripts/generate_coverage/merge_cobertura.py
[error] 10-10: Unable to import 'typer'
(E0401)
[error] 11-11: Unable to import 'plumbum.cmd'
(E0401)
[error] 12-12: Unable to import 'plumbum.commands.processes'
(E0401)
🪛 Ruff (0.11.9)
scripts/generate_coverage/run_rust.py
16-16: Boolean positional value in function call
(FBT003)
23-23: Boolean-typed positional argument in function definition
(FBT001)
23-23: Trailing comma missing
Add trailing comma
(COM812)
38-38: Boolean-typed positional argument in function definition
(FBT001)
55-55: Trailing comma missing
Add trailing comma
(COM812)
scripts/generate_coverage/merge_cobertura.py
15-15: Trailing comma missing
Add trailing comma
(COM812)
18-18: Trailing comma missing
Add trailing comma
(COM812)
33-33: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (7)
scripts/generate_coverage/set_outputs.py (1)
17-25: LGTM! Clean and straightforward implementation.The function correctly writes the coverage file path and format to the GitHub Actions output file. The implementation is simple and follows the established pattern used by other scripts in this suite.
scripts/generate_coverage/merge_cobertura.py (1)
23-38: LGTM! Excellent error handling and cleanup.The implementation correctly merges Cobertura files, handles command execution errors appropriately, and cleans up input files after successful merging. The use of typer's built-in file validation is a good improvement over custom validation logic.
scripts/generate_coverage/run_python.py (2)
22-35: LGTM! Well-structured helper function.The function properly returns different slipcover commands based on the format, with correct return type annotation. The logic for cobertura vs coveragepy formats is clear and appropriate.
56-57: Good use ofreplace()for file handling.Using
Path(".coverage").replace(out)correctly handles potential file overwrites, addressing the concern aboutFileExistsErrorthat could occur withrename().scripts/generate_coverage/run_rust.py (1)
44-60: LGTM! Solid coverage execution logic.The main function correctly handles mixed project file naming, creates output directories, executes cargo commands, and provides good error handling with detailed error messages.
scripts/generate_coverage/detect.py (2)
49-60: LGTM! Robust language detection logic.The function correctly detects project types based on the presence of
Cargo.tomlandpyproject.tomlfiles, handling all combinations appropriately including mixed projects.
70-83: Excellent format compatibility validation.The match/case logic comprehensively validates format compatibility between languages, correctly rejecting invalid combinations like coveragepy for Rust projects or lcov for Python projects. The constraint that mixed projects only support cobertura format is logical and well-enforced.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/generate_coverage/merge_cobertura.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/generate_coverage/merge_cobertura.py (4)
scripts/generate_coverage/detect.py (1)
main(63-83)scripts/generate_coverage/run_python.py (1)
main(38-60)scripts/generate_coverage/run_rust.py (1)
main(35-60)scripts/generate_coverage/set_outputs.py (1)
main(17-25)
🪛 Ruff (0.11.9)
scripts/generate_coverage/merge_cobertura.py
33-33: Trailing comma missing
Add trailing comma
(COM812)
🪛 Pylint (3.3.7)
scripts/generate_coverage/merge_cobertura.py
[error] 10-10: Unable to import 'typer'
(E0401)
[error] 11-11: Unable to import 'plumbum.cmd'
(E0401)
[error] 12-12: Unable to import 'plumbum.commands.processes'
(E0401)
🔇 Additional comments (1)
scripts/generate_coverage/merge_cobertura.py (1)
23-38: Well-implemented merge logic with proper error handling.The implementation correctly:
- Executes the merge command with proper error handling
- Writes the merged output to the specified path
- Cleans up input files after successful merge
- Provides clear error messages on failure
The destructive file deletion is appropriate given the documented behaviour in the function docstring.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
scripts/generate_coveragedocs/python-action-scripts.mdmatch/caseand helper functionsTesting
uvx ruff format scripts/generate_coverage/uvx ruff check --select I --fix scripts/generate_coverage/python -m py_compile scripts/generate_coverage/*.py scripts/ratchet_coverage/*.pyhttps://chatgpt.com/codex/tasks/task_e_68698603dffc8322b95501253dbb1d10
Summary by Sourcery
Convert the generate-coverage GitHub Action to use standalone Python helper scripts orchestrated via uv, replacing inline bash logic with reusable modules and improving readability and maintainability.
New Features:
Enhancements:
Documentation:
Chores: