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 output magic message when backend unavailable #1045

Merged
merged 4 commits into from Jan 10, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Member

jlstevens commented Jan 10, 2017

This PR addresses #999 regarding the inappropriate error message when trying to load an unavailable backend. Here is the old behavior:

image

And the new behavior:

image

I've tried to make a minimal fix. That said, the output message might suggest how to load the backend with notebook_extension...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 10, 2017

Now with a custom exception message:

image

I think this PR is ready to review/merge. I am happy to improve the exception message if you have other suggestions.

@jlstevens jlstevens requested a review from philippjfr Jan 10, 2017

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 10, 2017

What will happen if you ask for backend='powerpoint', i.e. one that truly doesn't exist? it would be nice if it were clear whether it's just nonsense/typo, or if there was a missing step. I understand that might not be practical, of course.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 10, 2017

Good suggestion! I don't think it would be too hard though I don't know if we have a list of all possible backend names somewhere I can use...

raise ValueError(OutputMagic.backend_list, value)
# raise ValueError("Backend %r not available. Has it been loaded with the notebook_extension?" % value)

This comment has been minimized.

@jbednar

jbednar Jan 10, 2017

Member

Stray comment?

This comment has been minimized.

@jlstevens

jlstevens Jan 10, 2017

Member

Thanks!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 10, 2017

With the last commit, you have two cases:

image

and

image

I think this PR is now ready for final review/merge.

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 10, 2017

Looks good.

@jbednar jbednar merged commit 642a604 into master Jan 10, 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.008%) to 76.851%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the backend_index_range_fix branch Jan 10, 2017

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