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

Fixed reraising of plotting exceptions #1158

Merged
merged 1 commit into from Feb 27, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Feb 27, 2017

This has been plaguing me for a long time. In python2 I always get these exceptions when there's an issue with the options. I think this is due to the nested try/excepts. Reraising an exception in this way differs slightly between py2 and py3 hence extra conditional.

TypeError: exceptions must be old-style classes or derived from BaseException, not NoneType
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 27, 2017

Appears there's some persistent py2 failure now, will have to look into it.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 27, 2017

I've always found reports of this error very mysterious - not only does the error message make no sense (all our classes derive from object, we don't have any old style classes!) but I've never been able to reproduce it myself.

Using raise to re-raise an exception is official, documented Python behavior so I don't know why it would cause issues. I am happy to merge a fix as long as the right traceback is displayed - ideally, it would just be a re-raised exception but that is what it is doing now and for some reason it has issues!

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 27, 2017

Using raise to re-raise an exception is official, documented Python behavior so I don't know why it would cause issues

It is if you're simply reraising, if you have another try/except in the same block you have to do what I did in the PR.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 27, 2017

I am happy to merge if you have checked the correct traceback is shown (which was a previous issue with this code that was fixed) and once the tests pass.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Feb 27, 2017

This problem has also bedeviled me, and I'm very glad there is a fix!

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 27, 2017

I've tested it on py2 and py3. The only thing that would be nice is to maybe shorten the traceback a bit, or rather show just the exception message for ValueErrors coming from param validation.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 27, 2017

The only thing that would be nice is to maybe shorten the traceback a bit, or rather show just the exception message for ValueErrors coming from param validation.

That can be implemented in a separate PR.Merging.

@jlstevens jlstevens merged commit 26feb9f into master Feb 27, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 78.191%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the reraise branch Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.