feat: Improve clang hook error diagnostics#226
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
==========================================
+ Coverage 96.70% 97.07% +0.36%
==========================================
Files 4 4
Lines 182 239 +57
==========================================
+ Hits 176 232 +56
- Misses 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
changed the title
WalkthroughThis PR refactors tool-version resolution and installation across clang-format and clang-tidy. The core change introduces ChangesVersion Resolution and Diagnostics Integration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/test_util.py (1)
338-341: ⚡ Quick winAvoid hardcoding resolved wheel patch version in test assertions.
This assertion will churn every time
CLANG_TIDY_VERSIONSadvances. Assert againstCLANG_TIDY_VERSIONS-derived expected value instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_util.py` around lines 338 - 341, The test currently asserts a hardcoded message containing the resolved wheel patch version; update the assertion to compute the expected message from CLANG_TIDY_VERSIONS instead of hardcoding "21.1.6". Locate the assertion in tests/test_util.py (the line checking that the string is in capsys.readouterr().err) and build the expected string using the CLANG_TIDY_VERSIONS mapping (or the derived value for the selected version) so the test compares capsys.readouterr().err against a message generated from CLANG_TIDY_VERSIONS rather than a fixed patch version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp_linter_hooks/util.py`:
- Around line 41-46: The _default_version_for_tool function currently returns
hardcoded constants (DEFAULT_CLANG_FORMAT_VERSION / DEFAULT_CLANG_TIDY_VERSION)
instead of the dependency-pinned defaults from pyproject.toml; change its
implementation to read the default versions from pyproject.toml (the same logic
used elsewhere for config loading) and return the clang-format or clang-tidy
value based on the tool argument, still falling back to the constants
DEFAULT_CLANG_FORMAT_VERSION and DEFAULT_CLANG_TIDY_VERSION only if the
pyproject values are missing or unparsable; update references in
_default_version_for_tool to use the pyproject-derived values so the function
satisfies the default-version contract.
- Around line 126-147: The code currently proceeds to call
_is_version_installed(tool, user_version) and _install_tool(tool, user_version)
even when resolve_tool_version returned user_version == None; add a guard after
resolve_tool_version(tool, version) to check if user_version is None and return
an explicit error (e.g., (None, "could not resolve default version for <tool>"))
instead of continuing; update the block around resolve_tool_version,
user_version, and the final return to early-return a diagnostic when
user_version is None so neither _is_version_installed nor _install_tool are
called with None.
---
Nitpick comments:
In `@tests/test_util.py`:
- Around line 338-341: The test currently asserts a hardcoded message containing
the resolved wheel patch version; update the assertion to compute the expected
message from CLANG_TIDY_VERSIONS instead of hardcoding "21.1.6". Locate the
assertion in tests/test_util.py (the line checking that the string is in
capsys.readouterr().err) and build the expected string using the
CLANG_TIDY_VERSIONS mapping (or the derived value for the selected version) so
the test compares capsys.readouterr().err against a message generated from
CLANG_TIDY_VERSIONS rather than a fixed patch version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e89bb289-ab74-47d3-a8fc-623aa5ae69d2
📒 Files selected for processing (6)
cpp_linter_hooks/clang_format.pycpp_linter_hooks/clang_tidy.pycpp_linter_hooks/util.pytests/test_clang_format.pytests/test_clang_tidy.pytests/test_util.py
| def _default_version_for_tool(tool: str) -> Optional[str]: | ||
| return ( | ||
| DEFAULT_CLANG_FORMAT_VERSION | ||
| if tool == "clang-format" | ||
| else DEFAULT_CLANG_TIDY_VERSION | ||
| ) |
There was a problem hiding this comment.
Default tool versions are not sourced from pyproject.toml.
_default_version_for_tool() still relies on constants backed by versions.py rather than dependency-pinned defaults from pyproject.toml, which breaks the required default-version contract.
As per coding guidelines, "Store default clang-format and clang-tidy versions in DEFAULT_CLANG_FORMAT_VERSION and DEFAULT_CLANG_TIDY_VERSION constants, reading from pyproject.toml".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp_linter_hooks/util.py` around lines 41 - 46, The _default_version_for_tool
function currently returns hardcoded constants (DEFAULT_CLANG_FORMAT_VERSION /
DEFAULT_CLANG_TIDY_VERSION) instead of the dependency-pinned defaults from
pyproject.toml; change its implementation to read the default versions from
pyproject.toml (the same logic used elsewhere for config loading) and return the
clang-format or clang-tidy value based on the tool argument, still falling back
to the constants DEFAULT_CLANG_FORMAT_VERSION and DEFAULT_CLANG_TIDY_VERSION
only if the pyproject values are missing or unparsable; update references in
_default_version_for_tool to use the pyproject-derived values so the function
satisfies the default-version contract.
| user_version, error = resolve_tool_version(tool, version) | ||
| if error is not None: | ||
| return None, error | ||
|
|
||
| if verbose: | ||
| if version is None: | ||
| print( | ||
| f"Using default {tool} Python wheel version {user_version}", | ||
| file=sys.stderr, | ||
| ) | ||
| elif version == user_version: | ||
| print(f"Using {tool} Python wheel version {user_version}", file=sys.stderr) | ||
| else: | ||
| print( | ||
| f"Resolved {tool} --version={version} to Python wheel version " | ||
| f"{user_version}", | ||
| file=sys.stderr, | ||
| ) | ||
|
|
||
| return ( | ||
| _is_version_installed(tool, user_version) or _install_tool(tool, user_version), | ||
| None, |
There was a problem hiding this comment.
Guard against unresolved default versions before install/check.
If user_version resolves to None, this path can flow into _is_version_installed() and trigger a runtime type error (None in result.stdout) when the binary exists. Return an explicit diagnostic instead of proceeding.
Suggested fix
def resolve_install_with_diagnostics(
tool: str, version: Optional[str], verbose: bool = False
) -> Tuple[Optional[Path], Optional[str]]:
@@
user_version, error = resolve_tool_version(tool, version)
if error is not None:
return None, error
+ if user_version is None:
+ return None, (
+ f"Could not determine a default {tool} wheel version from configuration."
+ )
@@
return (
_is_version_installed(tool, user_version) or _install_tool(tool, user_version),
None,
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp_linter_hooks/util.py` around lines 126 - 147, The code currently proceeds
to call _is_version_installed(tool, user_version) and _install_tool(tool,
user_version) even when resolve_tool_version returned user_version == None; add
a guard after resolve_tool_version(tool, version) to check if user_version is
None and return an explicit error (e.g., (None, "could not resolve default
version for <tool>")) instead of continuing; update the block around
resolve_tool_version, user_version, and the final return to early-return a
diagnostic when user_version is None so neither _is_version_installed nor
_install_tool are called with None.
Merging this PR will degrade performance by 19.25%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
1 participant
Summary
Improve first-run diagnostics for clang-format and clang-tidy hooks so adoption failures point users at concrete next steps.
Changes
--versionvalues and list supported wheel versions.--version=21.compile_commands.jsonis missing.cl.exe, and MSVC flag errors.Validation
uv run ruff check cpp_linter_hooks tests/test_util.py tests/test_clang_tidy.py tests/test_clang_format.pyuv run pytest tests/test_util.py tests/test_clang_tidy.py tests/test_clang_format.py -qSummary by CodeRabbit
Bug Fixes
New Features