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

BUG: Do not catch and silence KeyboardInterrupt #9129

Merged
merged 1 commit into from
Aug 11, 2018

Conversation

eric-wieser
Copy link
Contributor

Bare excepts are only correct when trying to capture and forward exceptions - in all other cases, except Exception should be used to avoid catching KeyboardInterrupt

This only touches the code that is user-facing, not any of the tooling or tests

Bare `except`s are only correct when trying to capture and forward exceptions - in all other cases, `except Exception` should be used to avoid catching `KeyboardInterrupt`

This only touches the code that is user-facing, not any of the tooling or tests
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Looks sensible at first glance. I restarted the stalled linux build. Mac OS failure unrelated.

I think I've also seen this done as:

    except KeyboardInterrupt:
        raise
    except:

@tylerjereddy tylerjereddy added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label Aug 10, 2018
@larsoner
Copy link
Member

 except KeyboardInterrupt:
        raise
    except:

IIRC this will also catch SystemExit, which is most often probably not what is desired.

@eric-wieser
Copy link
Contributor Author

I assume the test failure is unrelated?

@pv pv merged commit 1536d17 into scipy:master Aug 11, 2018
@pv pv added this to the 1.2.0 milestone Aug 11, 2018
@larsoner
Copy link
Member

Now that this is in, I think there is some style checker we can reconfigure to ensure we don't get new ones. I'll look into it unless someone else beats me to it. Should be a simple change.

@eric-wieser
Copy link
Contributor Author

@larsoner: Maybe, although I left all the ones alone in the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants