feat: add protocol for override hooks#1126
Conversation
📝 WalkthroughWalkthroughThis PR introduces a typed Protocol-based framework for override hooks with runtime signature validation. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
OverrideHookProtocol documents the interface for per-package override hooks and provides runtime validation of hook signatures via check_signature(). The lint command now checks override hook signatures. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Christian Heimes <cheimes@redhat.com>
![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_overrides.py (1)
57-69: 💤 Low value
test_check_signature_matchingduplicates coverage already provided by the parametrized test.
test_protocol_signature_matches_defaultalready validatescheck_signaturefor every hook (includingbuild_wheel) against its real default. This test adds value only if it's explicitly checking a hand-crafted override (not the default). Consider targeting an intentionally different-but-valid function to make the intent distinct, or remove it to reduce duplication.🤖 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_overrides.py` around lines 57 - 69, test_check_signature_matching currently duplicates coverage from test_protocol_signature_matches_default; either remove this redundant test or change it to exercise an explicit override function that differs from the real default but still matches the protocol. Concretely, update test_check_signature_matching to define a custom override (e.g., a purposely different-but-valid signature function for build_wheel with distinct parameter names or default values) and call overrides.OverrideHookProtocol.check_signature(your_custom_build_wheel) to assert the protocol accepts a non-default override, or simply delete test_check_signature_matching if no distinct behavior is needed.
🤖 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 `@src/fromager/overrides.py`:
- Around line 181-189: get_default currently lets a raw AttributeError bubble up
if the default function named by obj.fromager_default is missing; update
get_default (the classmethod named get_default and the use of
obj.fromager_default -> module_name, func_name) to catch AttributeError around
getattr(module, func_name) and raise a KeyError referencing the hook_name and
the missing func_name/module_name (include the original exception as the
__cause__ with "from") so callers get clear context about which hook/default is
broken.
In `@tests/test_overrides.py`:
- Around line 46-49: Remove the fragile hardcoded count check in
test_list_hooks: in the function test_list_hooks (which calls
overrides.OverrideHookProtocol.list_hooks) delete the assertion assert
len(hooks) == 13 and instead assert that hooks is non-empty (e.g., assert hooks)
and that there are no duplicates (e.g., assert len(set(hooks)) == len(hooks));
keep the isinstance(hooks, list) check and rely on the existing parametrized
test_protocol_signature_matches_default for per-hook behavior coverage.
---
Nitpick comments:
In `@tests/test_overrides.py`:
- Around line 57-69: test_check_signature_matching currently duplicates coverage
from test_protocol_signature_matches_default; either remove this redundant test
or change it to exercise an explicit override function that differs from the
real default but still matches the protocol. Concretely, update
test_check_signature_matching to define a custom override (e.g., a purposely
different-but-valid signature function for build_wheel with distinct parameter
names or default values) and call
overrides.OverrideHookProtocol.check_signature(your_custom_build_wheel) to
assert the protocol accepts a non-default override, or simply delete
test_check_signature_matching if no distinct behavior is needed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 783ce5ce-b387-4c9a-b237-19475396a74b
📒 Files selected for processing (3)
src/fromager/commands/lint.pysrc/fromager/overrides.pytests/test_overrides.py
| @classmethod | ||
| def get_default(cls, hook_name: str) -> typing.Callable[..., typing.Any]: | ||
| """Return the default function object for a hook name.""" | ||
| obj = getattr(cls, hook_name, None) | ||
| if obj is None or not hasattr(obj, "fromager_default"): | ||
| raise KeyError(hook_name) | ||
| module_name, func_name = obj.fromager_default | ||
| module = importlib.import_module(module_name) | ||
| return typing.cast(typing.Callable[..., typing.Any], getattr(module, func_name)) |
There was a problem hiding this comment.
get_default silently propagates AttributeError with no context.
If func_name doesn't exist in the imported module (e.g., typo in _default_hook args), getattr(module, func_name) raises a bare AttributeError with no mention of which hook is broken.
🛡️ Proposed fix
module = importlib.import_module(module_name)
- return typing.cast(typing.Callable[..., typing.Any], getattr(module, func_name))
+ func = getattr(module, func_name, None)
+ if func is None:
+ raise AttributeError(
+ f"Hook {hook_name!r}: no attribute {func_name!r} in {module_name!r}"
+ ) from None
+ return typing.cast(typing.Callable[..., typing.Any], func)🤖 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 `@src/fromager/overrides.py` around lines 181 - 189, get_default currently lets
a raw AttributeError bubble up if the default function named by
obj.fromager_default is missing; update get_default (the classmethod named
get_default and the use of obj.fromager_default -> module_name, func_name) to
catch AttributeError around getattr(module, func_name) and raise a KeyError
referencing the hook_name and the missing func_name/module_name (include the
original exception as the __cause__ with "from") so callers get clear context
about which hook/default is broken.
| def test_list_hooks() -> None: | ||
| hooks = overrides.OverrideHookProtocol.list_hooks() | ||
| assert isinstance(hooks, list) | ||
| assert len(hooks) == 13 |
There was a problem hiding this comment.
Hardcoded hook count breaks silently when hooks are added or removed.
assert len(hooks) == 13 will fail on every hook addition/removal without communicating which hook was added or removed. The parametrized test_protocol_signature_matches_default already exercises every hook end-to-end; this test adds little value in its current form.
♻️ Proposed fix
def test_list_hooks() -> None:
hooks = overrides.OverrideHookProtocol.list_hooks()
assert isinstance(hooks, list)
- assert len(hooks) == 13
+ # Spot-check a stable subset; the parametrized test validates all hooks.
+ assert "build_wheel" in hooks
+ assert "download_source" in hooks
+ assert len(hooks) > 0🤖 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_overrides.py` around lines 46 - 49, Remove the fragile hardcoded
count check in test_list_hooks: in the function test_list_hooks (which calls
overrides.OverrideHookProtocol.list_hooks) delete the assertion assert
len(hooks) == 13 and instead assert that hooks is non-empty (e.g., assert hooks)
and that there are no duplicates (e.g., assert len(set(hooks)) == len(hooks));
keep the isinstance(hooks, list) check and rely on the existing parametrized
test_protocol_signature_matches_default for per-hook behavior coverage.
There was a problem hiding this comment.
Another way to do this would be to have fromager define a type so we could pass fewer individual arguments to hooks. For example, the build hooks both take the same arguments. We could define a type and pass 1 value to the hook. Then when we add fields, the hooks don't have to change if they don't need the value.
Maybe the methods that manipulate the source (download, prepare, etc.) could be standardized to take a single type, too?
How many different types would we need?
2 participants
Pull Request Description
What
OverrideHookProtocol documents the interface for per-package override hooks and provides runtime validation of hook signatures via check_signature(). The lint command now checks override hook signatures.
Why