fix: move clang-tools to project dependencies section #118
Conversation
WalkthroughReads clang tool versions from the top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp_linter_hooks/util.py (1)
157-171: Guard: avoid installingtool==Nonewhen no version resolves.If both user input and defaults are
None,_install_toolwill attemptpip install clang-format==None. Add a guard and log.Apply this diff:
if user_version is None: user_version = ( DEFAULT_CLANG_FORMAT_VERSION if tool == "clang-format" else DEFAULT_CLANG_TIDY_VERSION ) - return _install_tool(tool, user_version) + if not user_version: + LOG.error("Could not resolve version for %s from input or project dependencies.", tool) + return None + return _install_tool(tool, user_version)
🧹 Nitpick comments (1)
cpp_linter_hooks/util.py (1)
31-33: Consider lazy-evaluating defaults to avoid import-time I/O.Reading and parsing
pyproject.tomlat import time can slow CLI startup and fails in wheel installs where the file isn’t present. Consider resolving defaults on demand or caching on first use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(1 hunks)pyproject.toml(2 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)
2-2: LGTM: build requirements look sane.
setuptools+setuptools-scmunder[build-system].requiresmatches your SCM versioning setup.If you rely on TOML configuration for setuptools-scm, confirm whether
setuptools-scm[toml]is recommended for your minimum supported versions.
37-39: Pinning clang tools under runtime deps aligns with util.py.Good move; exact pins match the curated version lists in
util.py.If publishing to PyPI, double-check these wheels exist for all supported Python versions/architectures.
cpp_linter_hooks/util.py
Outdated
| # Check dependencies | ||
| dependencies = data.get("dependencies", []) | ||
| for dep in dependencies: | ||
| if dep.startswith(f"{tool}=="): | ||
| return dep.split("==")[1] | ||
| return None |
There was a problem hiding this comment.
Fix: read from [project].dependencies (not top-level) and strip env markers.
pyproject.toml stores deps under [project].dependencies. Current lookup uses a missing top-level dependencies key, so defaults will always be None. Also, strip any ; ... markers before matching.
Apply this diff:
- # Check dependencies
- dependencies = data.get("dependencies", [])
- for dep in dependencies:
- if dep.startswith(f"{tool}=="):
- return dep.split("==")[1]
+ # Check [project].dependencies
+ dependencies = (data.get("project") or {}).get("dependencies", []) # PEP 621
+ for dep in dependencies:
+ dep_no_marker = dep.split(";", 1)[0].strip() # drop env markers/extras
+ if dep_no_marker.startswith(f"{tool}=="):
+ return dep_no_marker.split("==", 1)[1]Optional hardening (outside this hunk): add a fallback to installed metadata if pyproject.toml isn’t present at runtime.
# at top:
from importlib import metadata as _ilmd
# inside get_version_from_dependency(), before final `return None`:
try:
dist = _ilmd.distribution("cpp_linter_hooks")
for req in dist.requires or []:
req_no_marker = req.split(";", 1)[0].strip()
if req_no_marker.startswith(f"{tool}=="):
return req_no_marker.split("==", 1)[1]
except _ilmd.PackageNotFoundError:
pass🤖 Prompt for AI Agents
In cpp_linter_hooks/util.py around lines 23 to 28, the function currently reads
top-level "dependencies" (which is absent) and doesn't strip environment
markers, so it always returns None for pyproject.toml deps; modify it to read
data.get("project", {}).get("dependencies", []) instead of
data.get("dependencies", []), and when comparing split any marker by taking
dep.split(";", 1)[0].strip() before checking startswith(f"{tool}==") and
extracting the version. Optionally add the fallback: import importlib.metadata
as _ilmd at top and before returning None try to read installed distribution
requirements (catching _ilmd.PackageNotFoundError) and apply the same
marker-stripping logic to find the dependency version.
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 (1)
tests/test_util.py (1)
249-253: Set non-None defaults for clang tools (tests are failing).Update cpp_linter_hooks/util.py (lines 31–32) to fall back to the latest curated versions when get_version_from_dependency returns None:
DEFAULT_CLANG_FORMAT_VERSION = get_version_from_dependency("clang-format") or (CLANG_FORMAT_VERSIONS[-1] if CLANG_FORMAT_VERSIONS else None)
DEFAULT_CLANG_TIDY_VERSION = get_version_from_dependency("clang-tidy") or (CLANG_TIDY_VERSIONS[-1] if CLANG_TIDY_VERSIONS else None)
🧹 Nitpick comments (2)
tests/test_util.py (2)
58-59: Same issue in the “missing dependency” test.Align with [project].dependencies to avoid false positives.
- mock_toml_content = {"dependencies": ["other-package==1.0.0"]} + mock_toml_content = {"project": {"dependencies": ["other-package==1.0.0"]}}
25-45: Optional: add test for [project].dependencies and decide on backward-compatibilitycpp_linter_hooks/util.py uses tomllib.load and reads top-level "dependencies" via data.get("dependencies", []) (around lines 22–24) and does not inspect project -> dependencies. Add the suggested positive test for [project].dependencies and either support both shapes (parametrize tests) or remove top-level support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_util.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
tests/test_util.py
[error] 249-249: Step failed: coverage run --source=tests,cpp_linter_hooks -m pytest -vv. Test 'test_default_versions' failed with AssertionError: DEFAULT_CLANG_FORMAT_VERSION is None (tests/test_util.py:249).
⏰ 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)
tests/test_util.py (1)
28-33: Do not change the test mock without updating util; get_version_from_dependency reads top-leveldependencies.cpp_linter_hooks/util.py uses data.get('dependencies', []) (around lines 23–26) and tests in tests/test_util.py currently mock that shape; either update util to read project.dependencies (e.g. data.get('project', {}).get('dependencies', [])) and then apply your test diff, or leave the test unchanged.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the detection of clang-tools versions by moving them from the build dependencies to the project dependencies section in pyproject.toml. This ensures that the version detection utility can properly locate and read the tool versions from the correct section.
- Moved clang-format and clang-tidy from build-system.requires to project dependencies
- Updated the version detection utility to read from dependencies instead of build-system.requires
- Modified test cases to reflect the new dependency structure
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pyproject.toml | Moved clang-format and clang-tidy from build-system.requires to project dependencies |
| cpp_linter_hooks/util.py | Updated get_version_from_dependency to read from dependencies section instead of build-system.requires |
| tests/test_util.py | Updated test data to match new dependency structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
shenxianpeng
force-pushed
the
fix-deps
branch
from
e548e91 to
a212c74
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
- Coverage 94.44% 94.39% -0.06%
==========================================
Files 3 3
Lines 108 107 -1
==========================================
- Hits 102 101 -1
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #118 will not alter performanceComparing Summary
Footnotes
|
shenxianpeng
changed the title
Summary by CodeRabbit
Bug Fixes
Chores
Tests