skrawcz commented on code in PR #1524:
URL: https://github.com/apache/hamilton/pull/1524#discussion_r3005269728
##########
hamilton/function_modifiers/delayed.py:
##########
@@ -169,7 +169,10 @@ def resolve(self, config: dict[str, Any], fn: Callable) ->
NodeTransformLifecycl
for key in self._optional_config:
if key in config:
kwargs[key] = config[key]
- return self.decorate_with(**kwargs)
+ decorator = self.decorate_with(**kwargs)
+ if hasattr(decorator, "validate"):
+ decorator.validate(fn)
Review Comment:
Minor: Since `decorate_with` is typed as `Callable[...,
NodeTransformLifecycle]` and `NodeTransformLifecycle` always defines
`validate()`, the `hasattr` check will always be `True`. You could simplify to
just `decorator.validate(fn)`. That said, the defensive check is reasonable as
a guard against non-conforming lambdas, so this is just a nit — either way is
fine.
##########
tests/function_modifiers/test_delayed.py:
##########
@@ -155,5 +169,23 @@ def test_delayed_without_power_mode_fails():
with pytest.raises(base.InvalidDecoratorException):
decorator.resolve(
{"cols_to_extract": ["a", "b"], **CONFIG_WITH_POWER_MODE_DISABLED},
- fn=test_delayed_with_optional,
+ fn=fn,
)
+
+
+def test_dynamic_resolve_with_extract_fields():
+ """Test that @resolve with @extract_fields calls validate() correctly."""
+
+ def fn() -> dict[str, int]:
+ return {"a": 1, "b": 2}
+
+ decorator = resolve(
+ when=ResolveAt.CONFIG_AVAILABLE,
+ decorate_with=lambda fields: extract_fields(fields),
+ )
+ decorator_resolved = decorator.resolve(
+ {"fields": {"a": int, "b": int}, **CONFIG_WITH_POWER_MODE_ENABLED},
+ fn=fn,
+ )
+ assert hasattr(decorator_resolved, "resolved_fields")
+ assert decorator_resolved.resolved_fields == {"a": int, "b": int}
Review Comment:
Good test! One small thought: it might be worth adding a negative test case
that verifies `resolve()` properly propagates a `validate()` failure (e.g.,
passing a function with the wrong return type to `extract_fields`). This would
confirm the error path works correctly through the dynamic resolution. Not
blocking — the happy path coverage is sufficient for this fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
