Skip to content

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

Merged
gvanrossum merged 1 commit into
masterfrom
selectors-bare-except
Jun 10, 2017
Merged

bpo-30624 / selectors: use bare except clause#2082
gvanrossum merged 1 commit into
masterfrom
selectors-bare-except

Conversation

@giampaolo

@giampaolo giampaolo commented Jun 10, 2017

Copy link
Copy Markdown
Contributor

@mention-bot

mention-bot commented Jun 10, 2017

Copy link
Copy Markdown

@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

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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

gvanrossum commented Jun 10, 2017

Copy link
Copy Markdown
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

gvanrossum commented Jun 10, 2017

Copy link
Copy Markdown
Member

Needs a backport to 3.6.

serhiy-storchaka
serhiy-storchaka reviewed Jun 11, 2017
View reviewed changes
Comment thread Misc/NEWS
Library
-------

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

@serhiy-storchaka serhiy-storchaka Jun 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not SystemExit.

@giampaolo giampaolo Jun 11, 2017

Copy link
Copy Markdown
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.

@serhiy-storchaka serhiy-storchaka Jun 11, 2017

Copy link
Copy Markdown
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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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

giampaolo commented Jun 11, 2017

Copy link
Copy Markdown
Contributor Author

Look also at KqueueSelector.

What about it?

@serhiy-storchaka

serhiy-storchaka commented Jun 11, 2017

Copy link
Copy Markdown
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

giampaolo commented Jun 12, 2017

Copy link
Copy Markdown
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

gvanrossum commented Jun 12, 2017

Copy link
Copy Markdown
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

zware commented Jul 13, 2017

Copy link
Copy Markdown
Member

@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

giampaolo commented Jul 14, 2017

Copy link
Copy Markdown
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