-
Notifications
You must be signed in to change notification settings - Fork 283
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
Catch failed html_repr for cube #3376
Conversation
# and so fall back to the default cube repr. | ||
try: | ||
result = representer.repr_html() | ||
except (IndexError, ValueError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original preference was to catch any exceptions, but I see that's bad practice. Is this too broad a range of exceptions, too narrow, or just right?
try: | ||
result = representer.repr_html() | ||
except (IndexError, ValueError): | ||
result = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we warn that something's gone wrong? It might be weird to not get an html repr of a cube unexpectedly (my preference is no - Iris is loud enough already).
representer = CubeRepresentation(cube) | ||
|
||
# Confirm we get an exception when we make the html repr... | ||
with self.assertRaisesRegexp(ValueError, 'substring'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will stop erroring once #3373 is merged, so we will have to find / cause a different exception in the representer instead...
@DPeterK Fancy doing a rebase? |
8055e39
to
de51320
Compare
de51320
to
2d95e74
Compare
This seems to have gone stale. We still have ambitions for better HTML, particularly given @pp-mo's work turning the |
In jupyter, if an enhanced repr (such as
_repr_html_
) fails, the exception is raised but the execution continues, also returning the standard repr. This is useful for noting that there's a problem with the_repr_html_
, but very bad for UX (as reflected in #3351).As such, here we catch exceptions raised by constructing the html repr and explicitly return
None
so that the default repr is returned instead.