Skip to content

bpo-30624 / selectors: use bare except clause #2082

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 10, 2017
Merged

bpo-30624 / selectors: use bare except clause #2082

merged 1 commit into from
Jun 10, 2017

Conversation

giampaolo
Copy link
Contributor

@mention-bot
Copy link

@giampaolo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cf-natali, @1st1 and @serhiy-storchaka to be potential reviewers.

@giampaolo giampaolo mentioned this pull request Jun 10, 2017
Merged
gvanrossum
gvanrossum approved these changes Jun 10, 2017
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks! (Maybe we need an update to PEP 8 explaining that except: is okay when the block ends in bare raise?)

vstinner
vstinner approved these changes Jun 10, 2017
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't merge immediately because I would like to know if other core devs like this change.

@gvanrossum
Copy link
Member

Merging now so we can get this into the release candidate.

@gvanrossum gvanrossum merged commit 05dc20f into master Jun 10, 2017
@gvanrossum gvanrossum deleted the selectors-bare-except branch June 10, 2017 23:52
@gvanrossum
Copy link
Member

Needs a backport to 3.6.

serhiy-storchaka
serhiy-storchaka reviewed Jun 11, 2017
View reviewed changes
Misc/NEWS
@@ -350,6 +350,10 @@ Extension Modules
Library
-------

- bpo-30624: selectors does not take KeyboardInterrupt and SystemExit into
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not SystemExit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? SystemExit is not a subclass of Exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. My mistake.

serhiy-storchaka
serhiy-storchaka reviewed Jun 11, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look also at KqueueSelector.

@giampaolo
Copy link
Contributor Author

Look also at KqueueSelector.

What about it?

@serhiy-storchaka
Copy link
Member

Sorry, I though you fixed first two occurrences of "except Exception", but actually the 1st and the 3rd are fixed.

giampaolo added a commit that referenced this pull request Jun 11, 2017
@giampaolo
backport of bpo-30624 / PR #2082 for python 3.6
1fc5de9
@giampaolo giampaolo mentioned this pull request Jun 11, 2017
Merged
@giampaolo
Copy link
Contributor Author

Needs a backport to 3.6.

I see that on 3.6 except BaseException: is used, so the backport should not be necessary.

@gvanrossum
Copy link
Member

[For posterity, and note to myself] Oh, I see now what happened. Before #1035 this code was simpler and did have except BaseException: / unregister() / raise -- but @giampaolo changed that to except Exception: for some reason. It wasn't caught in review then (the PR makes it seem like a feature?) but I happened to mention it when reviewing #1030 (also by @giampaolo).

@zware
Copy link
Member

zware commented Jul 13, 2017

@giampaolo There is still a selectors-bare-except-3.6 branch in the repository that has no PR associated; does it contain something that still needs to be merged or can it be deleted?

@giampaolo
Copy link
Contributor Author

It can be deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@vstinner vstinner vstinner approved these changes

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@giampaolo @mention-bot @gvanrossum @serhiy-storchaka @zware @vstinner @Mariatta @the-knights-who-say-ni