tarmstrong / nbdiff

A diffing and merging tool for the IPython Notebook.
http://nbdiff.org
MIT License
212 stars 28 forks source link

Error handling for notebooks with invalid format #169

Closed selenaaa closed 10 years ago

selenaaa commented 10 years ago

Fixes #164

selenaaa commented 10 years ago

Thought it might be, but couldn't think of what to put.

Done.

selenaaa commented 10 years ago

Yeah but if modified_notebooks, only has one 'nbook' instead of multiple, then in that case if there is an error it should exit.

tarmstrong commented 10 years ago

I think more generally you want to quit if there are no successfully parsed notebooks. i.e., if there are two notebooks and neither can be parsed, quit.

selenaaa commented 10 years ago

Made the changes so now it shows the filename, only issue is, if there are multiple notebooks, and let's say one is invalid, nbdiff or nbmerge still opens, but the error message that prints to the console gets lost above all the get/post requests when the server starts

tarmstrong commented 10 years ago

refs #164

tarmstrong commented 10 years ago

Agreed about the Flask messages being a nuisance. See #170 .

I added some more example notebooks to the scripts/ folder with unparseable notebooks to more easily test this. Let's see how Travis likes them...

tarmstrong commented 10 years ago

We should do the same thing with filenames in nbdiff as well:

$ nbdiff before.ipynb after.ipynb
One or more of the files are not valid .ipynb files.
tarmstrong commented 10 years ago

The other issue here is that if you use nbmerge without arguments, it will do the right thing; if you specify the notebooks as arguments, it will just use the name of the last argument as the file name. E.g.,

# remote.ipynb is the invalid notebook as we can see by passing it through a json parser:
(py27)tavish@temeraire:~/capstone/nbdiff/scripts/example-notebooks/merge/2 (error_handling)$ python -m json.tool remote.ipynb
No JSON object could be decoded

# but local.ipynb is printed as part of the message:
(py27)tavish@temeraire:~/capstone/nbdiff/scripts/example-notebooks/merge/2 (error_handling)$ nbmerge local.ipynb base.ipynb remote.ipynb
There was a problem parsing the following notebook files:
local.ipynb
There are no valid notebooks to merge.
selenaaa commented 10 years ago

I feel like there might be a delay now when you run nbdiff. It works- not sure if it's just me, or in the case where there are multiple notebooks and some of them are invalid.

tarmstrong commented 10 years ago

The delay isn't your fault -- diffing is just slow.

tarmstrong commented 10 years ago

Made an issue for the performance problem: #173

tarmstrong commented 10 years ago

Looks good to me!