Fix @resolve decorator not calling validate() on returned decorators#1524
Fix @resolve decorator not calling validate() on returned decorators#1524jmarshrossney wants to merge 3 commits intoapache:mainfrom
@resolve decorator not calling validate() on returned decorators#1524Conversation
…on of extract_fields works; add unit test
… with valid annotations within the test body
skrawcz
left a comment
There was a problem hiding this comment.
Nice, clean bugfix! The root cause is well-identified: dynamically resolved decorators (via @resolve) skip the normal NodeTransformLifecycle.__call__() path where validate() is invoked, so decorators like @extract_fields that rely on validate() to set up internal state (e.g., resolved_fields) were broken.
The fix is minimal and correct — calling validate(fn) in resolve.resolve() before returning the decorator fills the lifecycle gap. Tests are well-structured and cover the specific failure case.
One minor suggestion below, otherwise LGTM. I'll approve and merge in a couple of days in case you want to address any of the comments first.
| return self.decorate_with(**kwargs) | ||
| decorator = self.decorate_with(**kwargs) | ||
| if hasattr(decorator, "validate"): | ||
| decorator.validate(fn) |
There was a problem hiding this 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.
| fn=fn, | ||
| ) | ||
| assert hasattr(decorator_resolved, "resolved_fields") | ||
| assert decorator_resolved.resolved_fields == {"a": int, "b": int} |
There was a problem hiding this 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.
|
Hi @skrawcz . Thanks for the quick and helpful feedback! I don't have a strong opinion on the "defensive check". If left up to me I'll leave it in with a note that makes exactly the point you just made. Tbh I'm still getting used to the idea that type annotations are in any way enforceable in a python project! More than happy to spend a little more time on tests. Might take a few days to get round to it though. Cheers! |
Hello!
This PR makes a trivial change to the delayed evaluation decorator (
@resolve) so that it works with decorators such as@extract_fieldswhich require theirvalidate()method to have been called. Concretely, the "fix" is to callvalidate()(if it is defined) inresolve.resolve, before returning it.Prior to this change, using
@resolvewith@extract_fieldsresults in anAttributeError: 'extract_fields' object has no attribute 'resolved_fields'.Changes
Mainly..
decorator.validate()before returning the decorator fromresolve.resolve.resolveandextract_fieldsspecificallyAlso..
test_dynamic_fails_without_power_mode_fails, and renamed ittest_dynamic_resolve_...How I tested this
Notes
Intended to fix #1409 - I have the same goals/issues.
This is my first contribution to this project. As a novice, the
CONTRIBUTING.mdguidelines could do with more detail! Note that the link to the developer setup guide appears broken.Checklist