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

Improved handling of errors in notebook_extension #1290

Merged
merged 1 commit into from Apr 13, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Apr 13, 2017

Better exception handling when loading backends using the notebook_extension.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

Looks good but could you mention exactly what the old behavior was and how the new behavior improve it?

In addition, I assume this doesn't fix the issue with the output magic where switching to matplotlib (when only bokeh has been loaded) doesn't issue an appropriate warning. Or does it? If not, it is something I wouldn't mind being included in this PR.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

Ok, I do see that it now handles ImportErrors differently from other exceptions and I agree that's useful. Does this PR do anything else?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 13, 2017

Looks good but could you mention exactly what the old behavior was and how the new behavior improve it?

Yes, sorry lost my comment somewhere. Basically this does two things that probably mostly affect developers.

  1. If a backend fails to import due to an Exception that's not an ImportError you get a warning with the original exception, usually this will happen when you've introduced a bug somewhere in the code and it would otherwise give you an uninformative error.

  2. If the matplotlib backend fails to import it now no longer throws an exception, (note this is not the same as making bokeh independent of matplotlib in general).

  3. If no backend imports there's now an exception.

@jbednar

This comment has been minimized.

Member

jbednar commented Apr 13, 2017

That all sounds fabulous!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 13, 2017

Looks good and I think this is very useful. Merging.

@jlstevens jlstevens merged commit d1481c3 into master Apr 13, 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 decreased (-0.01%) to 78.799%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the nb_load_handling branch Apr 19, 2017

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