nbconvert: Make sure node is atleast version 0.9.12

[jdfreder]

Trying to export html using nbconvert and an older version of node fails silently. All of the markdown gets stripped from the document. I used nvm to install and test node.js with our marked.js for node versions 0.6+. I discovered only node versions 0.9.12 and up produce any output. 0.8 and earlier produce no output and 0.9.11 and earlier hang.

This PR adds a test that makes sure that node is the right version before deciding to use it, otherwise pandoc is used.
#5263

[minrk]

See IPython.utils.version.check_version

[minrk]

This whole block can just be:

try:
    out, err, rc = get_output_error_code([cmd, '--version'])
except OSError:
    # command not found
    return False
if rc:
    # command failed
    return False
return check_version(out.lstrip('v'), '0.9.12')

[jdfreder]

@minrk I love the check_version function. I made the changes and pushed, what do you think of having the return_code on the same line as the check_version call (see checked in code).

[minrk]

what do you think of having the return_code on the same line as the check_version call (see checked in code).

Not what I would have done, but it's fine.

[minrk]

I realize there is one more thing missing in this fallback. In the case of neither node nor pandoc being present, the user will be presented with a 'pandoc missing' error, rather than recommending the preferred installation of 'node' for HTML conversion. I'm not certain we should address that in this PR, but it is suboptimal.

[takluyver]

It's not ideal that this is running subprocesses on import, either. But it was doing that before this change, so it's not critical to fix now.

[minrk]

I think we can merge this one. Maybe I'll take a pass at cleaning up both node and pandoc checking later if it keeps bothering me.

[minrk]

But it was doing that before

It was not launching node before, just locating it (which may have involved calling which).

[jdfreder]

I can add a warning to the else where node isn't found... something like

warnings.warn(  "Node.js 0.9.12 or later wasn't found.\n" +
                                "Nbconvert will try to use Pandoc instead.")

Also I can move the logic into a method that runs once and caches the results- so subprocess isn't called on import.

[minrk]

I can add a warning to the else where node isn't found

No, what we absolutely must not do is add a warning on import.

[jdfreder]

must not do is add a warning on import.

Is that a no to moving it into a method that runs once when the code is called too?

I was thinking

def markdown2html(source):
    """Convert a markdown string to HTML"""
    if _node[0] is None:
        # prefer md2html via marked if node.js >= 0.9.12 is available
        # node is called nodejs on debian, so try that first
        node_cmd = 'nodejs'
        if _verify_node(node_cmd):
            _node[0] = True
        else:
            node_cmd = 'node'
            if _verify_node(node_cmd):
                _node[0] = True
            else:
                warnings.warn(  "Node.js 0.9.12 or later wasn't found.\n" +
                                "Nbconvert will try to use Pandoc instead.")
                _node[0] = False
    if not _node[0]:
        return markdown2html_pandoc(source)
    else:
        return markdown2html_marked(source)

Pardon the [0] - scope issues.

[jdfreder]

Actually like this:

def markdown2html(source):
    """Convert a markdown string to HTML"""
    if _node[0] is None:
        # prefer md2html via marked if node.js >= 0.9.12 is available
        # node is called nodejs on debian, so try that first
        _node[0] = 'nodejs'
        if not _verify_node(_node[0] ):
            _node[0] = 'node'
            if not _verify_node(_node[0] ):
                warnings.warn(  "Node.js 0.9.12 or later wasn't found.\n" +
                                "Nbconvert will try to use Pandoc instead.")
                _node[0] = False
    if not _node[0]:
        return markdown2html_pandoc(source)
    else:
        return markdown2html_marked(source)

[minrk]

Something like that should be fine. Except for the _node[0] index access. Why would you do that?

[jdfreder]

. Why would you do that?

Because the method isn't child to a class, so I don't have a self to store the results in (variable assignment). I could do global _node instead, but I thought globals were generally a bad idea, right? Also I think the new local keyword is Python 3 only right?

[jdfreder]

... or we could just not cache the results, and run the check each time

[minrk]

_node is already a global. There is no benefit to making it a global container with one element rather than a singleton.

[jdfreder]

_node is already a global. There is no benefit to making it a global container with one element rather than a singleton.

Ahh okay, I was totally confused about the scope of global then. I thought it spanned across imports, but you're saying it only will span across imports if it is explicitly imported (i.e. from markdown import _node)?

[minrk]

global just means module-level

[jdfreder]

@takluyver and @minrk want to take a look again? I moved the code into a method and added a warning for the no-node case.

[takluyver]

Why not switch the if/else round and do if _node instead of if not _node?

[minrk]

+1 to switching this one around, otherwise ready to merge, I think.

[jdfreder]

Wasn't sure if there was ever a case where CBool(string) could == False... I guess I was just being overly cautious. Does that bother you? I can rotate them.

[takluyver]

An empty string booleates to False, but it would work the same whether you test s or not s.

[takluyver]

Yes, this no longer executes a subcommand on import. 👍

[minrk]

Great, this looks good to me now. Thanks, @jdfreder.

[jdfreder]

Order switched