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

Per cell exception #684

Merged
merged 8 commits into from Oct 18, 2017
Merged

Per cell exception #684

merged 8 commits into from Oct 18, 2017

Conversation

mpacer
Copy link
Member

@mpacer mpacer commented Oct 2, 2017

Closes #680.


def test_raises_exception_cell_tag(self):
"""
Check that conversion continues if ``cell_tags`` is False.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring doesn't seem quite right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

force_raise_errors = Bool(False,
help=dedent(
"""
If `False` (default), it does nothing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit clumsy to be adding another boolean config option which overrides an existing one, but I don't have a better idea right now.

The help string could be improved, however. "It does nothing" only makes sense if you know what the default behaviour is. I'd suggest something like:

If False (default), errors from executing the notebook can be allowed with a raises-exception tag on a single cell, or the allow_errors configurable option for all cells. An allowed error will be recorded in notebook output, and execution will continue. If an error occurs when it is not allowed, CellExecutionError will be raised.
If True, CellExecutionError will be raised for any error executing the notebook. This overrides both the allow_errors option and the raises-exception cell tag.

def test_raises_exception_cell_tag(self):
"""
Check that conversion continues if ``raises-exception`` is present in
the cell in which an exception is raised.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is actually testing force_raise_errors, though.

Can we split this into two tests: one with this name and docstring, and another one with this code (test_force_raise_errors)? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the test of it on its own implicitly occurs earlier in the global "convert everything!" test:

def test_run_notebooks(self):
"""Runs a series of test notebooks and compares them to their actual output"""
input_files = glob.glob(os.path.join(current_dir, 'files', '*.ipynb'))
for filename in input_files:
if os.path.basename(filename) == "Disable Stdin.ipynb":
continue
elif os.path.basename(filename) == "Interrupt.ipynb":
opts = dict(timeout=1, interrupt_on_timeout=True, allow_errors=True)
elif os.path.basename(filename) == "Skip Exceptions.ipynb":
opts = dict(allow_errors=True)
else:
opts = dict()
res = self.build_resources()
res['metadata']['path'] = os.path.dirname(filename)
input_nb, output_nb = self.run_notebook(filename, opts, res)
self.assert_notebooks_equal(input_nb, output_nb)

But you're right that this should explicitly test force_raise_errors instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, great. In that case, if you just want to fix the test name and docstring, I think this is ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants