-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
MAINT: test, refactor design of recursive closures #11910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f9c724e
to
75de762
Compare
I'm not sure this is an improvement for clarity, although good job finding the other cases. Thoughts, @ahaldane? Relates to https://bugs.python.org/issue4921, for anyone reading this who hasn't already found that. |
A possible alternative: class recursive(object):
def __init__(self, func):
self.func = func
def __call__(self, *args, **kwargs):
return self.func(self, *args, **kwargs) used as @recursive
def block_recursion(self, arrays, depth=0):
...
self(arrays[i], ...)
block_recursion(arrays) |
def recursive(f):
return f.__get__(f)
Edit: This doesn't work |
@@ -808,3 +808,11 @@ def _is_from_ctypes(obj): | |||
return 'ctypes' in ctype_base.__module__ | |||
except Exception: | |||
return False | |||
|
|||
|
|||
class recursive(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could do with a comment about why it is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an improvement to me. Let's wait for one more opinion, since @recursive
was my idea and might be an opinionated one.
I like this much more than the magical Perhaps a slight quibble is that I might call it something more specific, like |
|
||
''' | ||
def __init__(self, func): | ||
self.func = func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should probably call functools.update_wrapper
Whatever, there's little point updating the wrapper if its a local variable anyway. Thanks @mattip! |
In looking at
flake8 --select F811
local variable reuse, I stumbled across the solution in PR #10621 to the problem of recursive closures (#10620). It seems we can avoid assigning the closure to None by using the design pattern in the last comment to that PR.I also added a test for the use of recursive closures in
loadtxt
, which exposed another place where we still had cyclical references.