Minimal pandoc version warning #4680

dbarbeau · 2013-12-12T13:24:52Z

Follow-up of #4448

Adds a UserWarning informing about a bad Pandoc version. If pandoc version is < 1.12.1 the output of fenced markdown code blocks is not the expected one and precludes easy implementation of javascript highlighting in nbconvert.

minrk · 2013-12-13T00:38:57Z

Should probably make sure this works properly if no pandoc is available at all.

dbarbeau · 2013-12-13T14:23:18Z

Actually, Pandoc availability should be tested and properly reported a lot earlier I guess. I would do it as soon as, from the command line args, we're sure that pandoc is needed which is nearly always I think. But from reading the code I see a lot of effort has been done to keep things separated, so importing nbconvert.utils.pandoc from nbconvert.nbonvertapp doesn't look extra-clean.

As of the latest commit, the module import will not fail but issue warnings: "Pandoc not found" and "Bad Pandoc version". Exception raising is left to the pandoc(...) function. This is to not tie nbconvert's life to Pandoc's availability but still give some feedback to the user.

jdfreder · 2013-12-13T20:20:45Z

IPython/nbconvert/utils/pandoc.py

+    try:
+        out = subprocess.check_output(cmd, universal_newlines=True)
+        return True, None
+    except OSError, e:


Looks like Travis gets mad when testing against Python 3.3-

File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/IPython/nbconvert/utils/pandoc.py", line 66 except OSError, e:

I think you need to change this to except OSError as e:

dbarbeau · 2013-12-13T21:27:41Z

Thanks! Travis looks happy now :)

Carreau · 2013-12-15T18:12:20Z

I'm not a big fan in showing things at module import, but I guess it is fair here.

What do you think about showing it each time the pandoc(...) function is called ?

Will re-read and comment inline if anything else. Otherwise +1. Thanks !

Carreau · 2013-12-15T18:21:38Z

IPython/nbconvert/utils/pandoc.py

+
+def check_pandoc_version():
+    """Returns True if minimal pandoc version is met"""
+    return get_pandoc_version() >= minimal_version


String comparaison here. Is it wise ? Is it always the same as the meaning full? (see #2831 for background for example).

In [1]: '1.0.0' < '10.0.0' Out[1]: True In [2]: '2.0.0' < '10.0.0' Out[2]: False

string comparison shouldn't ever be used for versions anywhere. Use: IPython.utils.version.check_version.

dbarbeau · 2013-12-15T18:38:26Z

Ah nice! I don't know where I got that assumption from that version strings were comparable right away!
Having the warning inside the pandoc(...) function is better as far as I can see. I don't really like modules that yell at me when I import them either!

WIll update!

Carreau · 2013-12-15T18:48:49Z

Problem with warning every time is that it calls pandoc twice, which is not perfect.
You could cache the version and show the result in pandoc() every time, but then people could update pandoc and still show the (now inaccurate) message.

What about checking at each call if pandoc version is None or < min version and if >=min_version have a global that skip the ckeck ? That would make a compromise; that avoid to call pandoc at import time, and have a time penalty only first run and out-of-date pandoc system.

Carreau · 2013-12-18T13:11:09Z

Looks good, seem like the right balance !
WIll wait a few days to have the opinion of other dev and merge otherwise! Thanks !

damianavila · 2013-12-18T18:07:13Z

IPython/nbconvert/utils/pandoc.py

+
+    # if pandoc is missing let the exception bubble us out of here
+    pandoc_available(failmode="raise", alt=cmd)
+    check_pandoc_version()


what about:

if pandoc_available == True, None: check_version()

or something like that... and delete the try in line 138... because you are duplicating code in get_pandoc_version() throwing an PandocMissing after checking the version... again...

Wait, why don't I just call pandoc_available(...) from get_pandoc_version(), before line 131? Should simplify things no?

Yep... this is the other way...choose whatever you like 😉

…not honored in some situations. The IPython.nbconvert.utils.pandoc module prints a warning if minimal version is not satisfied..

…esent and issue warnings or raise exceptions when needed. Removed module root clutter and limited calling subprocess functions from too many places inside the module.

Put the pandoc version test under the pandoc_available test. This avoids raising an exception when the module is imported: the exception is only raised on pandoc(...) function calls. It also prints warnings if pandoc is not found at import time and if the version does not meet the expected one.

Improve error messages.

…sion is tested with IPython.utils.version.version_check

… it also tested for availability

- reduce code duplication - remove obscure parameters from pandoc_available - use find_cmd to locate binaries - group runtime cached values in a private dicitonnary (easier to manipulate) Add pandoc unittests.

… the end of the file

…s, less cached values.

damianavila · 2014-01-25T21:49:29Z

IPython/nbconvert/utils/pandoc.py

+        if not is_cmd_found('pandoc'):
+            raise PandocMissing()
+
+        out = subprocess.check_output( ['pandoc', '-v'], universal_newlines=True)


Why this block of code is indented? It is outside the if, so it is belong the the main def get_pandoc_version() function...

I may not have understood well, but the indented block belongs to the else clause. Double-checking... Yeah, it looks ok: if we have a cached version (global __version) return it, else go get it from pandoc's output, cache it and return it. I think that's it. That's what is intended at least.

OK, you are right... it was in my mind some previous versions of this code and I was thinking in that instead of reading with a new and clean view 😉 Thanks for the clarification.

Re-reading, I still think this can be written in another way:

if not is_cmd_found('pandoc'): raise PandocMissing() else: out = subprocess.check_output( ['pandoc', '-v'], universal_newlines=True) pv_re = re.compile(r'(\d{0,3}\.\d{0,3}\.\d{0,3})') __version = pv_re.search(out).group(0) return __version

I know that your implementation is working too, but (for me) it seems to fit better with the structure I propose you... but, at last, it is a matter of taste 😉

It can depend on general guidelines too like "no if without an else", but I often use this pattern when possible : put the checks (==with an early exit/raise/return) at the beginning of the block and treat the rest of the block as the "normal" track. A way to quickly see what is exceptional behaviour and what is normal. But I don't mind either way :)

As I said, it is a matter of taste... so I have no problems to keep it in you way 😉, unless another one raise your hand... thanks for your answers...

also added __init__ file to new utils test dir

jdfreder · 2014-01-28T18:40:41Z

Hi @dbarbeau

A made a couple of small changes and opened a PR against this PR (see https://github.com/dbarbeau/ipython/pull/1).

Usually we try to keep the tests next to the modules being tested (with the exception of the CasperJS based tests). I moved the pandoc tests into a new nbconvert/utils/test/ subdir.

Thanks for this feature, It's very useful- sorry it's taking so long to get it merged! When you merge my PR against yours, or have any questions, feel free to ping me.

EDIT: Removed a comment about the white space.

dbarbeau · 2014-01-28T19:44:34Z

Merged @jdfreder 's PR. Thanks for the help and for the tips concerning where to put tests! About the long merge period, well it takes the time it takes, I'm not in a hurry as long as things move forward ^^

jdfreder · 2014-01-28T21:05:33Z

👍 This looks good to merge

jdfreder · 2014-01-28T21:14:06Z

Since a lot of people have looked at this already, and it's assigned to me, I'll go ahead and merge it tonight if no one objects...

damianavila · 2014-01-28T21:27:06Z

I think is ready 👍

Minimal pandoc version warning

jdfreder · 2014-01-29T06:07:47Z

Thanks @dbarbeau for writing this!

damianavila · 2014-01-29T09:20:06Z

Yeah, thanks @dbarbeau and sorry for the long time (and the lot of rewriting...) 👍

takluyver · 2014-01-30T01:24:18Z

IPython/nbconvert/utils/tests/test_pandoc.py

+        original_minversion = pandoc._minimal_version
+
+        pandoc._minimal_version = "120.0"
+        assert not pandoc.check_pandoc_version()


We should suppress warnings here so it doesn't appear at the console.

http://docs.python.org/2/library/warnings.html#temporarily-suppressing-warnings

Minimal pandoc version warning

jdfreder reviewed Dec 13, 2013
View reviewed changes

Carreau reviewed Dec 15, 2013
View reviewed changes

damianavila reviewed Dec 18, 2013
View reviewed changes

dbarbeau mentioned this pull request Jan 25, 2014

Javascript hightlighting for NBConvert html output #4682

Merged

dbarbeau added 11 commits January 25, 2014 15:13

There is a bug in pandoc < 1.12.1 where the --no-highlight option is …

d5ce357

…not honored in some situations. The IPython.nbconvert.utils.pandoc module prints a warning if minimal version is not satisfied..

Adding checks to nbconvert.utils.pandoc module to ensure Pandoc is pr…

498059a

…esent and issue warnings or raise exceptions when needed. Removed module root clutter and limited calling subprocess functions from too many places inside the module.

Fix exception handler syntax for Python 3 compatibility.

e8aa302

Improve error messages.

pandoc version is now checked at each call to pandoc.pandoc(...). Ver…

42e936d

…sion is tested with IPython.utils.version.version_check

add whats new for minimal-pandoc

fbea645

move the pandoc_available(...) call inside get_pandoc_version(...) as…

58de7d4

… it also tested for availability

nbconvert.utils.pandoc:

800560e

- reduce code duplication - remove obscure parameters from pandoc_available - use find_cmd to locate binaries - group runtime cached values in a private dicitonnary (easier to manipulate) Add pandoc unittests.

code shuffling to present the API functions first, and the innards at…

cdd3d3c

… the end of the file

big cleanup of nbconvert.utils.pandoc module. Remove useless function…

966c01f

…s, less cached values.

fix Py3 test compatibility

fb80775

damianavila reviewed Jan 25, 2014
View reviewed changes

ghost assigned jdfreder Jan 26, 2014

jdfreder added 3 commits January 28, 2014 10:09

Fixed spacing in pandoc tests

48267b8

Move pandoc tests next to pandoc module

ae0347d

A couple more small changes,

3bcdc86

also added __init__ file to new utils test dir

fix test_pandoc imports

6871539

jdfreder added a commit that referenced this pull request Jan 29, 2014

Merge pull request #4680 from dbarbeau/minimal-pandoc

d0c6577

Minimal pandoc version warning

jdfreder merged commit d0c6577 into ipython:master Jan 29, 2014

takluyver reviewed Jan 30, 2014
View reviewed changes

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#4680 from dbarbeau/minimal-pandoc

678b619

Minimal pandoc version warning

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal pandoc version warning #4680

Minimal pandoc version warning #4680

dbarbeau commented Dec 12, 2013

minrk commented Dec 13, 2013

dbarbeau commented Dec 13, 2013

jdfreder Dec 13, 2013

dbarbeau commented Dec 13, 2013

Carreau commented Dec 15, 2013

Carreau Dec 15, 2013

minrk Dec 15, 2013

dbarbeau commented Dec 15, 2013

Carreau commented Dec 15, 2013

Carreau commented Dec 18, 2013

damianavila Dec 18, 2013

dbarbeau Dec 18, 2013

damianavila Dec 18, 2013

damianavila Jan 25, 2014

dbarbeau Jan 25, 2014

damianavila Jan 25, 2014

damianavila Jan 26, 2014

dbarbeau Jan 26, 2014

damianavila Jan 26, 2014

jdfreder commented Jan 28, 2014

dbarbeau commented Jan 28, 2014

jdfreder commented Jan 28, 2014

jdfreder commented Jan 28, 2014

damianavila commented Jan 28, 2014

jdfreder commented Jan 29, 2014

damianavila commented Jan 29, 2014

takluyver Jan 30, 2014

Minimal pandoc version warning #4680

Minimal pandoc version warning #4680

Conversation

dbarbeau commented Dec 12, 2013

minrk commented Dec 13, 2013

dbarbeau commented Dec 13, 2013

Choose a reason for hiding this comment

dbarbeau commented Dec 13, 2013

Carreau commented Dec 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbeau commented Dec 15, 2013

Carreau commented Dec 15, 2013

Carreau commented Dec 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdfreder commented Jan 28, 2014

dbarbeau commented Jan 28, 2014

jdfreder commented Jan 28, 2014

jdfreder commented Jan 28, 2014

damianavila commented Jan 28, 2014

jdfreder commented Jan 29, 2014

damianavila commented Jan 29, 2014

Choose a reason for hiding this comment