fix: move clang tools dependencies to optional#92
Conversation
shenxianpeng
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
- Coverage 95.65% 94.48% -1.17%
==========================================
Files 3 3
Lines 138 145 +7
==========================================
+ Hits 132 137 +5
- Misses 6 8 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cpp_linter_hooks/util.py(1 hunks)pyproject.toml(1 hunks)tests/test_util.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
get_version_from_dependency(16-30)
🔇 Additional comments (3)
pyproject.toml (1)
47-53: LGTM! Good architectural improvement.Moving clang tools to optional dependencies is a sound design decision. This allows users to install the core package without the heavyweight clang tools, and the clear comment provides good guidance for maintainers about what belongs in this section.
tests/test_util.py (2)
104-112: LGTM! Test structure updated correctly.The test data structure properly reflects the new optional-dependencies.tools format.
137-139: LGTM! Missing dependency test updated appropriately.The test correctly validates the behavior when no matching dependency is found in the tools section.
| @pytest.mark.benchmark | ||
| def test_get_version_from_dependency_fallback_to_dependencies(): | ||
| """Test get_version_from_dependency falls back to project.dependencies.""" | ||
| mock_toml_content = { | ||
| "project": { | ||
| "dependencies": [ | ||
| "clang-format==20.1.7", | ||
| "clang-tidy==20.1.0", | ||
| "other-package==1.0.0", | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| with ( | ||
| patch("pathlib.Path.exists", return_value=True), | ||
| patch("cpp_linter_hooks.util.tomllib.load", return_value=mock_toml_content), | ||
| ): | ||
| result = get_version_from_dependency("clang-format") | ||
| assert result == "20.1.7" | ||
|
|
||
| result = get_version_from_dependency("clang-tidy") | ||
| assert result == "20.1.0" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test expects fallback behavior that's missing in implementation.
This test validates fallback to the old dependencies structure, but the current implementation in cpp_linter_hooks/util.py doesn't include this fallback logic. The test will fail because the function only checks optional-dependencies.tools and returns None if not found there.
This test confirms that fallback functionality should exist but is missing from the implementation. Run this test to verify it fails:
🏁 Script executed:
#!/bin/bash
# Description: Run the specific fallback test to verify it fails due to missing implementation
python -m pytest tests/test_util.py::test_get_version_from_dependency_fallback_to_dependencies -vLength of output: 9788
🏁 Script executed:
#!/bin/bash
rg -n "def get_version_from_dependency" -A 20 -B 5 cpp_linter_hooks/util.pyLength of output: 1002
Implement fallback to project.dependencies in get_version_from_dependency
The test test_get_version_from_dependency_fallback_to_dependencies fails because the function only looks under project.optional-dependencies.tools and never falls back to project.dependencies. You’ll need to update cpp_linter_hooks/util.py to try the old dependencies list before returning None.
Locations to update:
• cpp_linter_hooks/util.py, inside get_version_from_dependency (around lines 23–30)
Suggested patch:
# First try project.optional-dependencies.tools
optional_deps = data.get("project", {}).get("optional-dependencies", {})
tools_deps = optional_deps.get("tools", [])
for dep in tools_deps:
if dep.startswith(f"{tool}=="):
return dep.split("==")[1]
+ # Fallback to project.dependencies
+ project_deps = data.get("project", {}).get("dependencies", [])
+ for dep in project_deps:
+ if dep.startswith(f"{tool}=="):
+ return dep.split("==")[1]
+
return NoneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cpp_linter_hooks/util.py around lines 23 to 30, the function
get_version_from_dependency currently only checks
project.optional-dependencies.tools for the package version and does not fall
back to project.dependencies. Update the function to first attempt to find the
version in project.optional-dependencies.tools, and if not found, then search in
project.dependencies before returning None. This ensures the fallback behavior
tested in tests/test_util.py lines 162 to 184 works correctly.
CodSpeed Performance ReportMerging #92 will degrade performances by 10.35%Comparing 🎉 Hooray!
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ❌ | test_run_clang_tidy_invalid[args4-1] |
757.9 µs | 845.3 µs | -10.35% |
| 🆕 | test_resolve_install_with_none_default_version |
N/A | 1 ms | N/A |
shenxianpeng
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores