Preview fails when first cell is raw

ReviewNB / support

Issues and feature requests for ReviewNB

https://reviewnb.com

59 stars 8 forks source link

Preview fails when first cell is raw #28

Closed slomeks closed 5 years ago

slomeks commented 5 years ago

When type of first cell in notebook is raw, rendering of changes fails.

amit1rrr commented 5 years ago

Sorry about that. We aren't explicitly handling raw cell types. Frankly, until today I didn't know they were part of the standard spec.

From the description it looks like raw cells are used only with nbconvert and not rendered in the Jupyter UI as such. Would it be fair for ReviewNB to not display any diffs for raw cells but rather handle raw cells gracefully so that the rest if the diff is rendered properly?

Also, if you could link me to the example PR/commit where diff is failing that would be useful. If it's a private repository and you don't mind me looking at it for debugging purpose then please email the url to amit@reviewnb.com

slomeks commented 5 years ago

Thanks for quick response!

Regarding visibility - I think raw cells should be visible as they are. According to spec you provided

A raw cell is defined as content that should be included unmodified in nbconvert output.

In our case they contain valuable metadata info. As such - we'd like to keep possibility to review them.

I'll provide example via email.

amit1rrr commented 5 years ago

I looked at the example you sent & the documentation. I'm not sure if it's even possible to show the visual diff for raw cell types because,

Users can define any format for raw cells - HTML, Latex, Python, RST, Markdown - or any other custom format. In case of your example there's no format defined in the metadata.
Without a defined format there's no way to render the cell and show visual diff.

We could fallback on textual diff for all raw cells but I don't know how useful that would be for Latex, HTML formats. In any case, my first priority is to make sure raw cells do not fail rest of the notebook diff.

Aside - Can you briefly tell me how/what are you using raw cells for in your workflow? This would help me a lot.

slomeks commented 5 years ago

We use it to store knowledge repo headers, as described in their docs.

Fully agree - prio should be not to break rendering rest of the notebook. Still, I think that it's better to see these cells. Otherwise they'll be hidden from user (even though they are visible in Jupyter and can potentially contain interesting stuff)

amit1rrr commented 5 years ago

Preview fails when first cell is raw

This is fixed now.

About raw cell diffs - Given the usage for these are pretty low and the effort to actually render diffs for even the most common raw cell types pretty high, I don't think it's a good idea to spend time on this feature at this stage.

slomeks commented 5 years ago

Hi Amit!

Just checked it and works fine - notebook renders properly.

Thanks a lot for your support!