-
Notifications
You must be signed in to change notification settings - Fork 57
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
Proxy JupyterHub 0.9 progress method to child #21
Conversation
@@ -129,6 +129,14 @@ def poll(self): | |||
else: | |||
return _yield_val(1) | |||
|
|||
if hasattr(Spawner, 'progress'): |
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.
I think it's fine to proxy the progress
method unconditionally. This will only ever be called if the Jupyterhub version supports it, and if so, the child spawner class is guaranteed to have at least a fallback .progress
method due to inheriting from the base Spawner
class.
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.
I went back and forth on the behavior several times, too. In theory we shouldn't have to worry about failures. But suppose someone is using an interactive debugger or uses a hasattr(cls, 'progress')
. An AttributeError makes sense if you try to access something that doesn't exist, but here we are sort of asserting that it exists. Mainy since this is a @property
and not function call, even accessing it, possibly other autocompletions and who knows what else, sees something that exists and then gives AttributeError when you try to access it.
There were enough possible special cases because it was @property
that I finally thought it simpler to condition the whole thing. Slightly uglier in a readable way, but many fewer possible surprises.
wrapspawner/wrapspawner.py
Outdated
if self.child_spawner: | ||
return self.child_spawner.progress | ||
else: | ||
return self.progress |
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.
I think this else
will hit a recursion limit. Instead, it should probably raise an error saying that it can't get progress when the child spawner doesn't exist.
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.
Thanks, a very good catch! I have updated the PR now.
Agreed to merge prior to releasing |
.progress()
method also #20Note: the case where the child_spawner does not exist yet is not handled (None will fail). Also the whole thing may need rethinking, but at least it solves my immediate problem!