-
Notifications
You must be signed in to change notification settings - Fork 123
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
Remove unecessary try/finally: in _multicall #277
base: main
Are you sure you want to change the base?
Conversation
In Python 3, every raisable thing inherits from BaseException, so the `except BaseException` in the code already handles every possible exit. So we can reduce the indentation and the slight overhead of `try`. Diff better viewed with `git diff -w`.
Hmm, though I like this cleanup I wonder if we should wait until #260 and #244 are discussed more. I'm wondering if we should be handling Also I guess we might run into trouble if Seems like a good idea on the surface for sure. |
Was just thinking about this change again for some odd reason and was thinking maybe we shouldn't be catching and packing The set of exceptions that derive from it is special:
Is there any reason to be catching anything but I'm pretty sure catching a |
pytest at least seems to rely on it. It has a couple of special control-flow BaseExceptions ( I haven't analyzed exactly where pytest relies on it, so it might be possible to adjust it not to rely on it. |
I feel like this might be somewhat ideal. As far as I grok deriving from As an example see |
Yeah the main thing that seems odd to me is catching any of these base exception types will likely have no value in result unboxing since they are all system level signals - the |
results.append(res) | ||
if firstresult: # halt further impl calls | ||
break | ||
except BaseException: |
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 I've already stated this, but just to re-iterate: IMO instead this PR should change this to Exception
and we shouldn't be handling BaseException
at all (unless we need it for GeneratorExit
- in which case we likely need a separate block anyway).
i'm deferring a merge of this until i understand the implications for async better, as a side-effect i might be ripping apart the loop anyway |
In Python 3, every raisable thing inherits from BaseException, so the
except BaseException
in the code already handles every possible exit. So we can reduce the indentation and the slight overhead oftry
.Diff better viewed with
git diff -w
(or add?w=1
in github).