Version conversion, support for X to Y even if Y < X (nbformat)

[jdfreder]

This is an implementation that is done in nbformat. I'm still not convinced that the conversion shouldn't be done in nbconvert once the ipynb->ipynb stuff gets checked in.

TODO

• Test
• Add tests

[jdfreder]

FWIW, I started out thinking I was going to implement this in nbconvert but later changed my mind. Consequently my local branch is named versionconversion_nbconvert (misleading, I know, sorry).

[minrk]

should be a relative import, I think

[jdfreder]

I tried a relative import, and it did not like my syntax. Did you mean something like from .reader import X, Y, etc?

[jdfreder]

@minrk I think my code comment (above) may have flew under your radar. I ended up using from a import b as c to resolve the naming conflict of reads in both classes.

[minrk]

generally use in (2,3) for this

[jdfreder]

Neat trick, thanks for showing me that

[minrk]

With the changes you have here, what would be the code for something like "open a notebook file, give it to me in the current format, noting the original format"?

[jdfreder]

With the changes you have here, what would be the code for something like "open a notebook file, give it to me in the current format, noting the original format"?

nb = reader.read(...) # will load the file as is
(major, minor) = reader.get_version(nb) # will give you the version 

That way the steps are separate, which is what you suggested, right? Whoever reads the notebook is responsible for keeping track of the version if it matters.

[minrk]

Sorry, by 'current version', I meant the latest version. That is, open the file, read it, upgrade it if necessary, note the original version.

[jdfreder]

Ah, like this:

nb = reader.read(...)
(major, minor) = reader.get_version(nb)
nb = convert.convert(nb, current.current_nbformat)

[jdfreder]

Tests added and comments addressed

[Carreau]

Why 16 in s[:16] ?

[jdfreder]

Just to display part of the JSON in the error message, that code came from current.py in master. Sorry it's kind of misleading since git has no way of tracking that I took those lines from that file.

See: https://github.com/ipython/ipython/blob/master/IPython/nbformat/current.py#L55

Just because it's in master doesn't mean it's the right behavior. It seems okay with me, do you think I should change it so it doesn't show any of the JSON?

[Carreau]

Then just a comment that say that 16 is "random". That the kind of things you can wonder when you came across that later, like, it might be to avoid new line, you might want `len('Notebook does not appear to be JSON:')+n < 80' to fit on terminal screen..

And for aestetic purpose add a '...' , I guess we can suppose we will never be in a case where the file is smaller than 16 char or empty.

[damianavila]

Overall look good... @minrk and @Carreau addressed the main issues, I just added two typo in comments.

[jdfreder]

Thanks all, comments addressed

[jdfreder]

@Carreau I just addressed your comment about the print JSON error length.

[minrk]

@ellisonbg do you want to have a look over this one to confirm that it's the way we want to go? Merging this is necessary to get started on the nbformat 4 work.

[ellisonbg]

Sure I will do that tomorrow...

On Sun, Oct 27, 2013 at 3:00 PM, Min RK notifications@github.com wrote:

@ellisonbg https://github.com/ellisonbg do you want to have a look over
this one to confirm that it's the way we want to go? Merging this is
necessary to get started on the nbformat 4 work.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4198#issuecomment-27180544
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[Carreau]

Detail when you will be working on nbformat v4, IJulia, IRuby, and soon IHaskell store notebook.metadata.language='haskell|ruby|julia' it would be great if the 3->4 converter was able to use that :-)

[ellisonbg]

OK I had a pretty close look at the core APIs in this PR. I think it looks good. I like the idea of having separate upgrade and downgrade functions for the two directions as the logic will be pretty different. I think that moving forward (not in this PR) we want to make sure our APIs define very clearly the difference between the in memory JSON versus the on disk JSON. That is not at all apparent in the to_notebook_json method right now. But overall I am +1 on merging this.

[minrk]

Okay, then merging away. Thanks!

[jdfreder]

Woohoo!