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

Improved handling of errors in notebook_extension #1290

Merged
merged 1 commit into from Apr 13, 2017
Merged

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Apr 13, 2017

Better exception handling when loading backends using the notebook_extension.

@jlstevens
Copy link
Contributor

@jlstevens 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
Copy link
Contributor

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

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

@jbednar jbednar commented Apr 13, 2017

That all sounds fabulous!

@jlstevens
Copy link
Contributor

@jlstevens 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
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
@philippjfr
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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants