fix(list-overrides): update stale and missing plugin hook names#1112
fix(list-overrides): update stale and missing plugin hook names#1112LalatenduMohanty wants to merge 2 commits intopython-wheel-build:mainfrom
Conversation
…details output Remove three hook names that never existed in the codebase (get_build_requirements, get_build_sdist_requirements, get_build_wheel_requirements) and add five hooks that were missing (get_build_system_dependencies, get_build_backend_dependencies, get_build_sdist_dependencies, get_install_dependencies_of_sdist, update_extra_environ). Closes: python-wheel-build#1107 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
📝 WalkthroughWalkthroughThis PR introduces two new module-level constants— Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_override_hook_names.py (1)
30-33: ⚡ Quick win
_collect_string_argmisses keyword-argument call sites.Only
node.argsis inspected, so a call likefind_and_invoke(req.name, method="build_wheel", default_fn=...)would not be picked up. Theextraassertion would then falsely flag"build_wheel"as unused and break the test.All current callers use positional args, so there's no live bug today—but the guard breaks silently if that convention changes.
🔧 Proposed fix — also scan keyword arguments
if len(node.args) > arg_index: arg = node.args[arg_index] if isinstance(arg, ast.Constant) and isinstance(arg.value, str): found.add(arg.value) + else: + # Fallback: check keyword arguments by parameter name position + # Build a mapping from keyword name to value for named parameters + # This handles calls like find_and_invoke(distname, method="hook_name", ...) + param_names = { + "find_and_invoke": ["distname", "method", "default_fn"], + "find_override_method": ["distname", "method"], + "_get_hooks": ["name"], + } + if name in param_names and arg_index < len(param_names[name]): + kw_name = param_names[name][arg_index] + for kw in node.keywords: + if kw.arg == kw_name and isinstance(kw.value, ast.Constant) and isinstance(kw.value.value, str): + found.add(kw.value.value)Alternatively, a simpler approach: scan
node.keywordsfor any keyword whose name matches the target param name, passing the param name as an additional argument to_collect_string_arg.🤖 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_override_hook_names.py` around lines 30 - 33, The helper _collect_string_arg only checks positional args (node.args), so keyword-call sites like method="build_wheel" are missed; update _collect_string_arg to also inspect node.keywords and add any ast.Constant string values whose keyword.arg matches the target parameter name (or, if using arg_index, accept an extra parameter for the param name and check keywords for that name), then add those string values to found just like positional args; reference node.args, node.keywords, arg_index, and _collect_string_arg when making the change.
🤖 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.
Nitpick comments:
In `@tests/test_override_hook_names.py`:
- Around line 30-33: The helper _collect_string_arg only checks positional args
(node.args), so keyword-call sites like method="build_wheel" are missed; update
_collect_string_arg to also inspect node.keywords and add any ast.Constant
string values whose keyword.arg matches the target parameter name (or, if using
arg_index, accept an extra parameter for the param name and check keywords for
that name), then add those string values to found just like positional args;
reference node.args, node.keywords, arg_index, and _collect_string_arg when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b597047f-4a09-48f5-839b-1a462cc04cff
📒 Files selected for processing (4)
src/fromager/commands/list_overrides.pysrc/fromager/hooks.pysrc/fromager/overrides.pytests/test_override_hook_names.py
ec53290 to
6897186
Compare
Move hardcoded hook names from list_overrides.py into GLOBAL_HOOK_NAMES (hooks.py) and OVERRIDE_HOOK_NAMES (overrides.py). Add AST-based tests that verify these constants match actual usage in the source tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
6897186 to
4d76425
Compare
Pull Request Description
Commit 1 (794fb39) — fix(list-overrides): update stale and missing plugin hook names
(get_build_system_dependencies, get_build_backend_dependencies, get_build_sdist_dependencies, get_install_dependencies_of_sdist, update_extra_environ).
output look less trustworthy.
Commit 2 (b573d40) — refactor(hooks): centralize hook name constants and add sync test
AST-based test that verifies these constants match actual usage in the source tree.
ensures any future hook addition or removal that isn't reflected in the constants will fail CI.
Closes: #1107