X Tutup
The Wayback Machine - https://web.archive.org/web/20221223163114/https://github.com/python/cpython/pull/2082
Skip to content
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

Jump to bottom

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

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

@giampaolo giampaolo commented Jun 10, 2017

@mention-bot
Copy link

mention-bot commented Jun 10, 2017

@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 changes
Copy link
Member

@gvanrossum gvanrossum left a comment

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 changes
Copy link
Member

@vstinner vstinner left a comment

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

@gvanrossum
Copy link
Member

gvanrossum commented Jun 10, 2017

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 Jun 10, 2017
@gvanrossum
Copy link
Member

gvanrossum commented Jun 10, 2017

Needs a backport to 3.6.

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

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

@serhiy-storchaka serhiy-storchaka Jun 11, 2017

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

@giampaolo giampaolo Jun 11, 2017

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

@serhiy-storchaka serhiy-storchaka Jun 11, 2017

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 changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Look also at KqueueSelector.

@giampaolo
Copy link
Contributor Author

giampaolo commented Jun 11, 2017

Look also at KqueueSelector.

What about it?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 11, 2017

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

giampaolo commented Jun 12, 2017

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

gvanrossum commented Jun 12, 2017

[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

giampaolo commented Jul 14, 2017

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

@vstinner vstinner

@gvanrossum gvanrossum

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

Successfully merging this pull request may close these issues.

None yet
8 participants
@giampaolo @mention-bot @gvanrossum @serhiy-storchaka @zware @vstinner @Mariatta @the-knights-who-say-ni
X Tutup