jmarshrossney commented on code in PR #1524:
URL: https://github.com/apache/hamilton/pull/1524#discussion_r3032698840
##########
tests/function_modifiers/test_delayed.py:
##########
@@ -189,3 +189,64 @@ def fn() -> dict[str, int]:
)
assert hasattr(decorator_resolved, "resolved_fields")
assert decorator_resolved.resolved_fields == {"a": int, "b": int}
+
+
+def test_resolve_from_config_with_extract_fields():
+ """Test @resolve_from_config with @extract_fields calls validate()
correctly."""
+
+ def fn() -> dict[str, int]:
+ return {"a": 1, "b": 2}
+
+ decorator = resolve_from_config(
+ 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}
+
+
+def test_resolve_propagates_validate_failure():
+ """Test that validate() failures are propagated through resolve."""
+
+ def fn() -> str:
+ return "not what you were expecting..."
+
+ decorator = resolve(
+ when=ResolveAt.CONFIG_AVAILABLE,
+ decorate_with=lambda fields: extract_fields(fields),
+ )
+ with pytest.raises(base.InvalidDecoratorException):
+ decorator.resolve(
+ {"fields": {"a": int, "b": int}, **CONFIG_WITH_POWER_MODE_ENABLED},
+ fn=fn,
+ )
+
+
+def test_resolve_with_arbitrary_decorator():
+ """Test behavior when decorate_with returns something that is not a
NodeTransformLifecycle."""
+
+ # NOTE: do we actually want this test to pass?
+
+ # A decorator that doesn't inherit from NodeTransformLifecycle (but still
uses kwargs only)
+ class ArbitraryDecorator:
+ def __init__(self, a: int, b: int) -> None:
+ pass
+
+ def __call__(self, f: Callable) -> Callable:
+ return f
+
+ def fn() -> pd.DataFrame:
+ return pd.DataFrame()
+
+ decorator = resolve(
+ when=ResolveAt.CONFIG_AVAILABLE,
+ decorate_with=lambda kwargs: ArbitraryDecorator(**kwargs),
+ )
+ decorator_resolved = decorator.resolve(
+ {"kwargs": {"a": 1, "b": 2}, **CONFIG_WITH_POWER_MODE_ENABLED},
+ fn=fn,
+ )
+ assert isinstance(decorator_resolved, ArbitraryDecorator)
Review Comment:
I'm not sure that it's a good thing for this test to pass (which it
currently does).
I guess the way I see it (with the caveat that I'm pretty unfamiliar with
the Hamilton codebase) is
- Pro: the way decorators are/aren't checked is the same whether or not they
have been wrapped in `@resolve`
- Con: there is an missed opportunity to check decorator validity earlier
on, inside `resolve.resolve()`, which might be preferable than failing in a
confusing way later on
Concretely, we could add the following in `resolve.resolve()`
```python
if not isinstance(decorator, NodeTransformLifecycle):
raise InvalidDecoratorException(
f"decorate_with must return a NodeTransformLifecycle, got
{type(decorator).__name__}"
)
```
and remove the `hasattr` check.
--
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]
