Skip to content

fix(list-overrides): update stale and missing plugin hook names#1112

Draft
LalatenduMohanty wants to merge 2 commits intopython-wheel-build:mainfrom
LalatenduMohanty:fix/update-hook-names-1107
Draft

fix(list-overrides): update stale and missing plugin hook names#1112
LalatenduMohanty wants to merge 2 commits intopython-wheel-build:mainfrom
LalatenduMohanty:fix/update-hook-names-1107

Conversation

@LalatenduMohanty
Copy link
Copy Markdown
Member

@LalatenduMohanty LalatenduMohanty commented May 4, 2026

Pull Request Description

  • Commit 1 (794fb39) — fix(list-overrides): update stale and missing plugin hook names

    • What: Removes 3 hook names that never existed (get_build_requirements, get_build_sdist_requirements, get_build_wheel_requirements) and adds 5 that were missing
      (get_build_system_dependencies, get_build_backend_dependencies, get_build_sdist_dependencies, get_install_dependencies_of_sdist, update_extra_environ).
    • Why: The list-overrides --details output was incomplete — plugins implementing these 5 hooks wouldn't show them, and the 3 fake names were dead weight that made the
      output look less trustworthy.
  • Commit 2 (b573d40) — refactor(hooks): centralize hook name constants and add sync test

    • What: Moves the hook name list out of list_overrides.py into canonical constants (GLOBAL_HOOK_NAMES in hooks.py, OVERRIDE_HOOK_NAMES in overrides.py) and adds an
      AST-based test that verifies these constants match actual usage in the source tree.
    • Why: The stale names in commit 1 existed because the list was hand-written with no mechanism to detect drift. The constants give a single source of truth, and the test
      ensures any future hook addition or removal that isn't reflected in the constants will fail CI.

Closes: #1107

…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>
@LalatenduMohanty LalatenduMohanty requested a review from a team as a code owner May 4, 2026 20:59
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

📝 Walkthrough

Walkthrough

This PR introduces two new module-level constants—GLOBAL_HOOK_NAMES in hooks.py and OVERRIDE_HOOK_NAMES in overrides.py—to centralize the enumeration of supported hook names. The list_overrides.py command is refactored to use these constants instead of a hardcoded hook list. A new test file employs AST parsing to enforce that these constants remain synchronized with actual hook-name usage across the codebase.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description clearly relates to the changeset: it documents the removal of stale hook names, addition of missing ones, and the refactoring to centralize hook constants with a synchronization test.
Title check ✅ Passed The title accurately describes the main change: fixing and updating plugin hook names in the list-overrides command by deriving them from centralized constants instead of hardcoded values.

✏️ 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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify Bot added the ci label May 4, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_override_hook_names.py (1)

30-33: ⚡ Quick win

_collect_string_arg misses keyword-argument call sites.

Only node.args is inspected, so a call like find_and_invoke(req.name, method="build_wheel", default_fn=...) would not be picked up. The extra assertion 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.keywords for 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

📥 Commits

Reviewing files that changed from the base of the PR and between bc300d4 and b573d40.

📒 Files selected for processing (4)
  • src/fromager/commands/list_overrides.py
  • src/fromager/hooks.py
  • src/fromager/overrides.py
  • tests/test_override_hook_names.py

@LalatenduMohanty LalatenduMohanty marked this pull request as draft May 4, 2026 21:07
@LalatenduMohanty LalatenduMohanty force-pushed the fix/update-hook-names-1107 branch 3 times, most recently from ec53290 to 6897186 Compare May 4, 2026 22:28
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>
@LalatenduMohanty LalatenduMohanty force-pushed the fix/update-hook-names-1107 branch from 6897186 to 4d76425 Compare May 4, 2026 22:35
@LalatenduMohanty LalatenduMohanty changed the title Fix/update hook names 1107 fix(list-overrides): update stale and missing plugin hook names May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

list-overrides: update stale and missing plugin hook names in --details output

1 participant