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

Restore behavior of the "raises-exception" cell tag #7020

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@joelostblom
Copy link
Contributor

@joelostblom joelostblom commented Aug 15, 2019

This restores the intended behavior of the raises-exception cell tag. Details can be found in #7015.

Close #7015

@vidartf Let me know if there are more changes required.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 15, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@vidartf
Copy link
Member

@vidartf vidartf commented Aug 15, 2019

All tests pass, but I won't be able to give this a spin this week (the binder doesn't have working kernels). For reviewer (non collaborators can also review): A simple test is to make two cells and run with "Run all":

5 / 0

and

5 + 5

The second cell should run with output, and the first cell get traceback for the zero division.

@jasongrout jasongrout added this to the 1.1 milestone Aug 16, 2019
Copy link
Contributor

@jasongrout jasongrout left a comment

@vidartf, @joelostblom - are we sure we want to include all the cell metadata in an execute request? It seems there would be a lot of superfluous stuff in there. Perhaps we can just include the things we expect to be meaningful to an execute request?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

I see that the output execution method is looking into this metadata to find this tag. But then all metadata is being passed on to the execution. This seems loose, passing on all sorts of irrelevant things into the actual execution message.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

@blink1073 and I discussed this. The difficult thing here is that we want to tell some things to the output execute function, and some things to the kernel, and right now we are combining those things in the output execute arguments.

So let's merge this for now as a bugfix that we can backport to 1.0.x. Then in a separate PR we can introduce a new argument option to the output execute function for this information, and then we can explicitly talk to the output execute function vs the kernel message.

@blink1073 blink1073 merged commit 7d0b052 into jupyterlab:master Aug 16, 2019
9 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 16, 2019

@meeseeksdev backport to 1.0.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Aug 16, 2019
blink1073 added a commit that referenced this issue Aug 16, 2019
…0-on-1.0.x

Backport PR #7020 on branch 1.0.x (Restore behavior of the "raises-exception" cell tag)
@joelostblom
Copy link
Contributor Author

@joelostblom joelostblom commented Aug 16, 2019

Thanks for merging so quickly!

@joelostblom joelostblom deleted the restore-raises-exception branch Aug 16, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants