fix importing notebooks #2569
fix importing notebooks #2569
Conversation
|
@marckruzik I still see a problem as the markdown cell will be added below the cell executing the Can you describe the scenario you are after? |
I saw that markdown cells are causing an error display, so I supposed (maybe wrongly) that markdown should be displayed as well. If markdown is not displayed, that's fine by me. |
| [Theory] | ||
| [InlineData(".ipynb")] | ||
| [InlineData(".dib")] | ||
| public async Task It_imports_and_forwards_markdown_content_to_frontend(string notebookExt) |
There was a problem hiding this comment.
I'm not sure I understand the test name here. What if there is no frontend?
| } | ||
| else | ||
| { | ||
| await kernel.RootKernel.SendAsync(new SubmitCode(element.Contents, element.KernelName)); |
There was a problem hiding this comment.
A bunch of repeated logic in the if blocks. These can probably be hoisted up to the switch or else maybe switch is inappropriate.
|
I think this is the right shape: @marckruzik what are your thoughts? |
clear handling markdown cells as output
| var @event = new DisplayedValueProduced(element.Contents, context.Command, new[] | ||
| { | ||
| new FormattedValue("text/markdown", element.Contents), | ||
| new FormattedValue(PlainTextFormatter.MimeType, element.Contents) |
There was a problem hiding this comment.
I don't see the plain text output being tested, and I'm not convinced this is the correct value for it. I could make the argument that the plain text version should remove the formatting characters from the Markdown, e.g. convert it to HTML and then grab the inner text from the HTML.
| { | ||
| var document = await InteractiveDocument.LoadAsync( | ||
| notebookFile, | ||
| CreateKernelInfos(kernel.RootKernel as CompositeKernel)); | ||
|
|
||
| foreach (var element in document.Elements) | ||
| { | ||
| await kernel.RootKernel.SendAsync(new SubmitCode(element.Contents, element.KernelName)); | ||
| switch (element.KernelName?.ToLowerInvariant()) |
There was a problem hiding this comment.
At this point this should be based on the language name, not the kernel name.
There was a problem hiding this comment.
Correct, will do this in a new pr
importing notebooks with markdown cells adds those to the ui.
This will add all markdown cells after the cell executing the import and all outputs that the imported notebook will generate