Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So you are changing the behaviour here -- now you will intercept BaseExceptions. Is that intended? What if a
SystemExitoccurs and you override it with some exception thatunregistermight rise?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 just checked the #2082 pr. FWIW I think that the correct code would be this:
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'm not sure I understand. Exceptions on Python 3 are chained, so in case of error on
unregisterthe previous exception will still be shown. As such, there's no need of thetry/finally.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'm guessing it matters if the original exception was
SystemExit,unregisterwere to raise e.g.KeyError, and there was aKeyErrorexception handler active -- even though exceptions are chained, without thefinallyit would still raise an instance ofKeyErrorwhich would be caught, rather than raisingSystemExitwhich would not be caught. Then again such aKeyErrorhandler would have to be considered overly broad (if it could catch exceptions from aselector.register()call).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 is very uncommon code. If it is needed we have much more general problem. Every
except ... raiseshould be rewritten in similar way. Maybe even everyfinallyblock. This is too cumbersome and may even need syntax support or changing the semantic.Uh oh!
There was an error while loading. Please reload this page.
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.
In general it is needed when you catch BaseExceptions. That's why you don't want to catch them at all usually.
For this particular code, I think @Haypo is right. I looked through the
unregistermethod implementations and it looks like they don't raise exceptions (or can have unexpected ones). So probably a bareexcept/raiseshould work here.