fix: move clang tools dependencies to optional #92
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate limit exceeded@shenxianpeng has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 20 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 (3)
## Walkthrough
The changes update how dependency versions for tools are retrieved. The lookup now prioritizes the `tools` group under `project.optional-dependencies` in `pyproject.toml` instead of the main dependencies list. Tests and configuration are adjusted accordingly.
## Changes
| File(s) | Change Summary |
|-------------------------|-------------------------------------------------------------------------------------------------------------|
| cpp_linter_hooks/util.py | Modified `get_version_from_dependency` to look for tool versions in `optional-dependencies.tools` before falling back to `dependencies`. Added fallback to hardcoded versions if resolved version is None in `_resolve_install`. |
| pyproject.toml | Moved `clang-format` and `clang-tidy` from main dependencies to `optional-dependencies.tools`; added clarifying comment restricting this group to clang tools only. |
| tests/test_util.py | Updated tests to reflect new `optional-dependencies.tools` structure and added a test verifying fallback to hardcoded versions when default constants are None. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Test
participant Util
participant PyprojectToml
Test->>Util: get_version_from_dependency(tool)
Util->>PyprojectToml: Read pyproject.toml
alt tool version in optional-dependencies.tools
Util->>PyprojectToml: Look up tool in optional-dependencies.tools
alt tool found
Util-->>Test: Return version
else tool not found
Util->>PyprojectToml: Look up tool in dependencies
alt tool found
Util-->>Test: Return version
else
Util-->>Test: Return None
end
end
else
Util->>PyprojectToml: Look up tool in dependencies
alt tool found
Util-->>Test: Return version
else
Util-->>Test: Return None
end
endPossibly related PRs
Suggested labels
|
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.
tests/test_util.py
Outdated
Comment on lines
162
to
184
| @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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores