fix: simplify tool installation and invocation #113
Conversation
github-actions
bot
added
the
documentation
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 28 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 (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSwitched pre-commit file selection from path-based regexes to language-type filters; simplified installation utilities by removing public install/query helpers and always invoking installs for side effects; updated clang wrapper modules to call literal Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cpp_linter_hooks/util.py (4)
214-221: Bug: unconditional warning + breaking return type; restore return value and fix logging.
LOG.warningexecutes even when the tool is found, and changing the return type toNonebreaks callers/tests.-def ensure_installed(tool: str, version: Optional[str] = None) -> None: - """Ensure a tool is installed, resolving its version if necessary.""" - LOG.info("Ensuring %s is installed", tool) - tool_path = _resolve_install(tool, version) - if tool_path: - LOG.info("%s available at %s", tool, tool_path) - LOG.warning("%s not found and could not be installed", tool) +def ensure_installed(tool: str, version: Optional[str] = None) -> Optional[Path]: + """Ensure a tool is installed, resolving its version if necessary. Returns the resolved executable path or None.""" + LOG.info("Ensuring %s is installed (requested=%s)", tool, version or "auto") + tool_path = _resolve_install(tool, version) + if tool_path: + LOG.info("%s available at %s", tool, tool_path) + return tool_path + LOG.error("%s not found and could not be installed", tool) + return None
177-204: Avoid repeat installs, handle None versions safely, and prefer existing tools unless explicitly pinned.When
versionis not provided, we should not reinstall; also choose a sane install target when the tool is missing. This addresses the repeat‑install issue and the pipeline failures withversion=Noneor'20'.def _resolve_install(tool: str, version: Optional[str]) -> Optional[Path]: - """Resolve the installation of a tool, checking for version and installing if necessary.""" - user_version = _resolve_version( - CLANG_FORMAT_VERSIONS if tool == "clang-format" else CLANG_TIDY_VERSIONS, - version, - ) - if user_version is None: - user_version = ( - DEFAULT_CLANG_FORMAT_VERSION - if tool == "clang-format" - else DEFAULT_CLANG_TIDY_VERSION - ) - - path = shutil.which(tool) - if path: - runtime_version = _get_runtime_version(tool) - if runtime_version and user_version not in runtime_version: - LOG.info( - "%s version mismatch (%s != %s), reinstalling...", - tool, - runtime_version, - user_version, - ) - return _install_tool(tool, user_version) - return Path(path) - - return _install_tool(tool, user_version) + """Resolve installation, only reinstalling when an explicit version is requested.""" + versions = CLANG_FORMAT_VERSIONS if tool == "clang-format" else CLANG_TIDY_VERSIONS + requested_version = _resolve_version(versions, version) if version is not None else None + default_version = ( + (DEFAULT_CLANG_FORMAT_VERSION if tool == "clang-format" else DEFAULT_CLANG_TIDY_VERSION) + or (versions[-1] if versions else None) + ) + + path = shutil.which(tool) + if path: + # No explicit version requested: trust the existing tool to avoid repeated installs. + if requested_version is None: + return Path(path) + runtime_version = _get_runtime_version(tool) + if runtime_version and requested_version not in runtime_version: + LOG.info("%s version mismatch (%s != %s), reinstalling...", tool, runtime_version, requested_version) + return _install_tool(tool, requested_version) + # If we can't detect the runtime version, assume OK and avoid reinstall. + return Path(path) + + # Tool not found: choose a version to install. + install_version = requested_version or default_version + if not install_version: + LOG.error("No candidate version available to install for %s", tool) + return None + return _install_tool(tool, install_version)
165-175: After pip install, resolve the binary robustly; PATH lookup can fail immediately.
shutil.which(tool)can returnNoneright after install in some CI shells. Add a fallback to search next tosys.executable.def _install_tool(tool: str, version: str) -> Optional[Path]: """Install a tool using pip.""" try: - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] - ) - return shutil.which(tool) + subprocess.check_call( + [sys.executable, "-m", "pip", "install", "--disable-pip-version-check", f"{tool}=={version}"] + ) + path = shutil.which(tool) + if path: + return Path(path) + # Fallback: look next to the current interpreter (handles PATH quirks in CI) + bin_dir = Path(sys.executable).parent + candidates = [bin_dir / tool] + if sys.platform.startswith("win"): + candidates.insert(0, bin_dir / f"{tool}.exe") + for p in candidates: + if p.exists(): + return p + return None except subprocess.CalledProcessError: LOG.error("Failed to install %s==%s", tool, version) return None
151-161: Runtime version parsing is brittle; parse semver with a regex for both tools.Current tidy parsing assumes a second line; format parsing uses the last token. Use a single regex to extract
\d+(\.\d+)+.Example helper (apply outside this hunk):
import re _VERSION_RE = re.compile(r"\b(\d+(?:\.\d+)+)\b") def _get_runtime_version(tool: str) -> Optional[str]: try: out = subprocess.check_output([tool, "--version"], text=True) m = _VERSION_RE.search(out) return m.group(1) if m else None except Exception: return NoneREADME.md (1)
177-187: Use--verbose(remove-v) — clang-format has no-vshort alias.
Update README.md (lines 177–187): remove the-vsuggestion and recommend--verboseonly.
🧹 Nitpick comments (2)
README.md (1)
57-59: Clarify how version resolution works.Minor wording tweak to set expectations for major-only pins resolving to the latest patch.
> [!TIP] - > Install the latest version of `clang-format` and `clang-tidy` if not specified. You can specify the version using the `--version` argument in the `args` list as shown below. + > Install the latest version of `clang-format` and `clang-tidy` if not specified. You can pin versions via `--version`. A major-only value (e.g., `--version=21`) resolves to the latest `21.x.y`; use a full version (e.g., `21.1.0`) for reproducible builds.cpp_linter_hooks/util.py (1)
38-40: Default version discovery via pyproject may be absent in wheels.When running inside a packaged environment,
pyproject.tomloften isn't shipped. Ensure you always have a fallback (you now do via the proposed_resolve_installchange), or consider defaultingDEFAULT_*to the last known version at import time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.pre-commit-hooks.yaml(1 hunks)README.md(1 hunks)cpp_linter_hooks/clang_format.py(1 hunks)cpp_linter_hooks/clang_tidy.py(1 hunks)cpp_linter_hooks/util.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cpp_linter_hooks/clang_format.py (1)
cpp_linter_hooks/util.py (1)
ensure_installed(214-220)
cpp_linter_hooks/clang_tidy.py (1)
cpp_linter_hooks/util.py (1)
ensure_installed(214-220)
🪛 Ruff (0.12.2)
cpp_linter_hooks/clang_tidy.py
15-15: Consider ["clang-tidy", *other_args] instead of concatenation
Replace with ["clang-tidy", *other_args]
(RUF005)
🪛 GitHub Actions: Test
cpp_linter_hooks/util.py
[error] 216-220: cpp_linter_hooks.util: ensure_installed() returned None for clang-format when no version was specified (version=None).
[error] 216-220: cpp_linter_hooks.util: ensure_installed() returned None for clang-format with version '20' (version=20).
[error] 216-220: cpp_linter_hooks.util: ensure_installed() returned None for clang-tidy when no version was specified (version=None).
[error] 216-220: cpp_linter_hooks.util: ensure_installed() returned None for clang-tidy with version '20' (version=20).
[error] 216-220: cpp_linter_hooks.util: ensure_installed() returned None when tool not found (tool='clang-format').
[error] 216-220: cpp_linter_hooks.util: version mismatch handling for clang-format; runtime=18.1.8 vs requested=20.1.7; installation attempted but tool name not returned.
[error] 216-220: cpp_linter_hooks.util: cannot determine runtime version for clang-format; ensure_installed('clang-format', '20.1.7') returned None (runtime_version=None).
🔇 Additional comments (2)
.pre-commit-hooks.yaml (1)
6-6: Switch to types_or looks good; header extensions should still match.
types_or: [c++, c]is appropriate and typically covers headers (.h,.hpp, etc.) via identify tags.If you have Objective‑C(++) or CUDA/Proto in this repo, consider extending types:
types_or: [c, c++, objective-c, objective-c++, cuda, proto].Also applies to: 14-14
README.md (1)
75-76: Verify migration notes link exists in the published package.At runtime (inside a pre‑commit venv),
docs/migration-notes.mdmay not be present if it isn’t packaged. Ensure the link is valid on GitHub or include it in the sdist/wheel.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (2)
pyproject.toml (2)
36-36: Verify necessity of setuptools as a runtime dependency.No pkg_resources matches were found — confirm whether setuptools is required at runtime. Run these checks locally:
- git grep -n --heading -e '\bpkg_resources\b' || true
- git grep -n -e 'importlib.resources|importlib.metadata' || true
If pkg_resources is only used to access package data/metadata, migrate to importlib.resources / importlib.metadata and remove setuptools from runtime deps.
49-53: Relax exact pins for thetoolsextra to a stable major range.Exact == pins cause churn and redundant reinstalls; prefer major-locked ranges to get patch fixes without breaking changes.
[project.optional-dependencies] # only clang tools can added to this section to make hooks work tools = [ - "clang-format==21.1.0", - "clang-tidy==21.1.0", + "clang-format>=21.1,<22", + "clang-tidy>=21.1,<22", ]Confirm CI matrix and README/examples still behave with the relaxed pins (check .github/workflows/test.yml and .github/workflows/codspeed.yml); also verify tests/docs that reference pinned wheels (tests/test_util.py, docs/migration-notes.md).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp_linter_hooks/util.py (2)
165-175: _install_tool returns str but is annotated as Optional[Path].Type inconsistency can propagate subtle bugs. Return a Path consistently.
def _install_tool(tool: str, version: str) -> Optional[Path]: @@ - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] - ) - return shutil.which(tool) + subprocess.check_call([sys.executable, "-m", "pip", "install", f"{tool}=={version}"]) + path = shutil.which(tool) + return Path(path) if path else None
190-201: Normalize runtime/user versions and compare exact normalized versions (avoid substring containment).Replace the check in cpp_linter_hooks/util.py (lines 190–201) — currently
if runtime_version and user_version not in runtime_version:— with a normalized equality check (e.g., strip build metadata after '-' or '(', or parse into integer tuples) and compare normalized values; update tests that assume substring behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp_linter_hooks/util.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: pre-commit
cpp_linter_hooks/util.py
[error] 215-218: End-of-file-fixer hook (pre-commit) modified the file; pre-commit exited with code 1. Re-run 'pre-commit run --all-files' to apply fixes.
🪛 GitHub Actions: Test
cpp_linter_hooks/util.py
[error] 1-1: During pytest run (coverage run --source=tests,cpp_linter_hooks -m pytest -vv), ensure_installed() returned None instead of the tool name for clang-format/clang-tidy, causing 7 parametrized tests to fail.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (1)
cpp_linter_hooks/util.py (1)
215-218: Run pre-commit or commit formatting for cpp_linter_hooks/util.py (lines 215–218)
End-of-file-fixer changed this hunk — likely trailing whitespace or missing final newline. Install and run pre-commit locally and commit the formatting change (pip install pre-commit && pre-commit run --all-files) or ensure the file ends with a single final newline.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp_linter_hooks/util.py (2)
145-154: Type mismatch: returning str where Optional[Path] is annotated.shutil.which returns Optional[str], not Path. This causes typing drift and confuses callers.
Apply this diff:
-def _install_tool(tool: str, version: str) -> Optional[Path]: +def _install_tool(tool: str, version: str) -> Optional[Path]: @@ - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] - ) - return shutil.which(tool) + subprocess.check_call( + [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] + ) + which = shutil.which(tool) + return Path(which) if which else None
157-170: None version can reach pip; add idempotency and clear failure.If DEFAULT_* is None, _install_tool is called with None (TypeError). Also, every call triggers a pip invocation, which risks the repeat-install loop described in #112.
Apply this diff to (a) guard against None, (b) skip re-install when already installed and unchanged within the same process, and (c) fail fast with a clear error:
+_LAST_INSTALLED: dict[str, Optional[str]] = {} + def _resolve_install(tool: str, version: Optional[str]) -> Optional[Path]: @@ - if user_version is None: - user_version = ( - DEFAULT_CLANG_FORMAT_VERSION - if tool == "clang-format" - else DEFAULT_CLANG_TIDY_VERSION - ) + if user_version is None: + user_version = ( + DEFAULT_CLANG_FORMAT_VERSION + if tool == "clang-format" + else DEFAULT_CLANG_TIDY_VERSION + ) + if user_version is None: + raise RuntimeError( + f"No version available for {tool}. " + "Set --version or pin it in build-system.requires." + ) + + # Idempotency within a single process to avoid repeat installs + if _LAST_INSTALLED.get(tool) == user_version: + which = shutil.which(tool) + return Path(which) if which else None @@ - return _install_tool(tool, user_version) + installed = _install_tool(tool, user_version) + if installed: + _LAST_INSTALLED[tool] = user_version + return installedcpp_linter_hooks/clang_tidy.py (1)
21-29: Capture stderr too; clearer error reporting.clang-tidy may emit diagnostics on stderr. Combine streams for consistent detection and messaging.
Apply this diff:
- try: - sp = subprocess.run(command, stdout=subprocess.PIPE, encoding="utf-8") + try: + sp = subprocess.run(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8") retval = sp.returncode - output = sp.stdout + output = (sp.stdout or "") + (sp.stderr or "") if "warning:" in output or "error:" in output: retval = 1 return retval, output except FileNotFoundError as stderr: retval = 1 return retval, str(stderr)
♻️ Duplicate comments (2)
cpp_linter_hooks/util.py (1)
170-170: Restore public ensure_installed for backward compatibility (fixes CI).Tests (and likely users) import ensure_installed; its removal causes ImportError in the pipeline. Reintroduce it as a thin wrapper and mark as deprecated.
Apply this diff below the existing functions:
+def ensure_installed(tool: str, version: Optional[str] = None) -> str: + """ + Ensure `tool` is installed at the requested version. + Returns the executable path (str). Raises on failure. + Note: kept for backward compatibility; prefer _resolve_install or future public API. + """ + LOG.info("Ensuring %s is installed", tool) + installed = _resolve_install(tool, version) + if installed: + return str(installed) + which = shutil.which(tool) + if which: + return which + raise RuntimeError(f"Failed to install {tool}=={version or 'unspecified'}")Run to confirm there are no remaining imports of removed symbols and that tests pass collection:
#!/bin/bash rg -nP '\bfrom\s+cpp_linter_hooks\.util\s+import\b.*\bensure_installed\b' -C2 rg -nP '\bimport\s+cpp_linter_hooks\.util\b' -C2 pytest -q --maxfail=1 -k utilcpp_linter_hooks/clang_tidy.py (1)
14-16: Fail fast on install errors; use splat args.Check installation result and adopt Ruff’s splat suggestion.
Apply this diff:
- if hook_args.version: - _resolve_install("clang-tidy", hook_args.version) - command = ["clang-tidy"] + other_args + tool_path = "clang-tidy" + if hook_args.version: + tool_path = ensure_installed("clang-tidy", hook_args.version) + command = [tool_path, *other_args]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp_linter_hooks/clang_format.py(2 hunks)cpp_linter_hooks/clang_tidy.py(2 hunks)cpp_linter_hooks/util.py(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- cpp_linter_hooks/clang_format.py
🧰 Additional context used
🧬 Code graph analysis (1)
cpp_linter_hooks/clang_tidy.py (1)
cpp_linter_hooks/util.py (1)
_resolve_install(157-170)
🪛 GitHub Actions: Test
cpp_linter_hooks/util.py
[error] 1-1: Missing symbol 'ensure_installed' in cpp_linter_hooks.util (ImportError during test collection).
🪛 Ruff (0.12.2)
cpp_linter_hooks/clang_tidy.py
16-16: Consider ["clang-tidy", *other_args] instead of concatenation
Replace with ["clang-tidy", *other_args]
(RUF005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
- Coverage 94.48% 94.44% -0.04%
==========================================
Files 3 3
Lines 145 108 -37
==========================================
- Hits 137 102 -35
+ Misses 8 6 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the clang tools installation mechanism to prevent repeated installations and simplifies the tool invocation process. The changes move clang-format and clang-tidy declarations from optional dependencies to build-system requirements and streamline the installation logic.
- Moved clang tool declarations from
project.optional-dependencies.toolstobuild-system.requiresin pyproject.toml - Simplified tool installation by removing version checking and only installing when a specific version is requested
- Removed complex
ensure_installedandis_installedfunctions in favor of direct tool execution with conditional installation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Moves clang-format and clang-tidy from optional dependencies to build-system requirements |
| cpp_linter_hooks/util.py | Removes version checking logic and simplifies installation to only occur when specific versions are requested |
| cpp_linter_hooks/clang_format.py | Updates to use simplified installation logic and direct tool execution |
| cpp_linter_hooks/clang_tidy.py | Updates to use simplified installation logic and direct tool execution |
| tests/test_util.py | Removes tests for deleted functions and updates remaining tests for new logic |
| .pre-commit-hooks.yaml | Changes file selection from regex patterns to language types |
| README.md | Adds tip about tool installation behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ): | ||
| result = _resolve_install("clang-format", "20.1.7") | ||
| assert result == Path(mock_path) | ||
| assert Path(result) == Path(mock_path) |
There was a problem hiding this comment.
The test expects a Path object but _resolve_install now returns the result of _install_tool, which returns Optional[Path] from shutil.which. This assertion will fail if shutil.which returns a string instead of a Path object.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
CodSpeed Performance ReportMerging #113 will improve performances by ×2.3Comparing Summary
Benchmarks breakdown
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp_linter_hooks/util.py (1)
145-156: Invalid requirement when version is None + return type mismatch.
- When version is None, f"{tool}=={version}" becomes "tool==None" (pip error).
- Function advertises Optional[Path] but returns Optional[str] from shutil.which.
Apply this diff to handle None safely and align types:
-def _install_tool(tool: str, version: str) -> Optional[Path]: - """Install a tool using pip, suppressing output.""" +def _install_tool(tool: str, version: Optional[str]) -> Optional[str]: + """Install a tool using pip, suppressing output.""" try: - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - ) - return shutil.which(tool) + req = f"{tool}=={version}" if version else tool + subprocess.check_call( + [sys.executable, "-m", "pip", "install", req, "--disable-pip-version-check", "--no-input"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + return shutil.which(tool) except subprocess.CalledProcessError: LOG.error("Failed to install %s==%s", tool, version) return Nonetests/test_util.py (1)
135-137: Fix failing assertion to match suppressed pip output.The implementation redirects stdout/stderr to DEVNULL; assert accordingly (this is the current CI failure).
Apply this diff:
- mock_check_call.assert_called_once_with( - [sys.executable, "-m", "pip", "install", "clang-format==20.1.7"] - ) + mock_check_call.assert_called_once_with( + [sys.executable, "-m", "pip", "install", "clang-format==20.1.7"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + )
♻️ Duplicate comments (2)
cpp_linter_hooks/util.py (2)
159-172: Always re-installing causes repeat installs; short-circuit when possible.Currently this unconditionally installs, re-triggering pip even when the tool is already available, which contradicts the PR goal. Short-circuit when the tool is already on PATH and no explicit version is requested; also update the signature type to match _install_tool.
Apply this diff:
-def _resolve_install(tool: str, version: Optional[str]) -> Optional[Path]: +def _resolve_install(tool: str, version: Optional[str]) -> Optional[str]: """Resolve the installation of a tool, checking for version and installing if necessary.""" + # Fast path: if no version requested and tool already exists, avoid pip + existing = shutil.which(tool) + if version is None and existing: + return existing @@ - return _install_tool(tool, user_version) + return _install_tool(tool, user_version)Optional (nice-to-have): keep a per-process dedupe cache to avoid repeated installs of the same (tool, version) tuple when multiple callers invoke this in a single run. I can provide a patch if you want it.
23-29: Fragile default version sourcing; add safe fallback path.Reading versions from build-system.requires in pyproject.toml at runtime is brittle (pyproject.toml may not be packaged in wheels), and returning None cascades into an invalid pip requirement later. Add a fallback to the latest known supported version when no pinned default is discoverable.
Apply this diff (pairs with the _resolve_install change below):
def _resolve_install(tool: str, version: Optional[str]) -> Optional[Path]: @@ - if user_version is None: - user_version = ( - DEFAULT_CLANG_FORMAT_VERSION - if tool == "clang-format" - else DEFAULT_CLANG_TIDY_VERSION - ) + if user_version is None: + user_version = ( + DEFAULT_CLANG_FORMAT_VERSION + if tool == "clang-format" + else DEFAULT_CLANG_TIDY_VERSION + ) + # Fallback to the latest known supported version when no default is pinned + if user_version is None: + user_version = ( + CLANG_FORMAT_VERSIONS[-1] + if tool == "clang-format" + else CLANG_TIDY_VERSIONS[-1] + )
🧹 Nitpick comments (2)
cpp_linter_hooks/util.py (1)
148-152: Ruff S603 (subprocess) — risk is low but document the guardrails.Inputs are constrained (no shell, versions resolved from curated lists or pinned defaults). Add a brief comment to silence future concerns.
- subprocess.check_call( + # Safe: shell=False, args fully controlled; version comes from curated list/defaults. + subprocess.check_call(tests/test_util.py (1)
168-177: Optional: assert we don’t hit pip when tool is already present and no version is requested.Once the short-circuit in _resolve_install lands, consider adding a variant of this test with version=None and asserting subprocess.check_call is not invoked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(2 hunks)tests/test_util.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
tests/test_util.py
[error] 135-135: test_install_tool_success failed: mocked subprocess.check_call invoked with stdout=DEVNULL and stderr=DEVNULL; expected call did not include these arguments. Command: 'coverage run --source=tests,cpp_linter_hooks -m pytest -vv'.
🪛 Ruff (0.12.2)
cpp_linter_hooks/util.py
148-148: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (2)
tests/test_util.py (2)
28-35: LGTM: tests now target build-system.requires.
60-60: LGTM: missing dependency returns None as expected.
shenxianpeng
changed the title
shenxianpeng
added
minor
github-actions
bot
added
bug
shenxianpeng
removed
documentation
github-actions
bot
added
the
documentation
|
shenxianpeng
changed the title
shenxianpeng
removed
the
documentation
shenxianpeng
changed the title
closes #112 and #95
Summary by CodeRabbit
Documentation
Chores
Refactor
Tests