fix for an issue with empty math block

[pacahon]

#726

[mpacer]

So, it looks like your expected output for the modified test does not match the output that we should expect — which makes me think we need to reëvaluate that test, because it's not raising the error you mentioned in #726.

[pacahon]

@mpacer Not sure I got it. I added smoke test to make sure AttributeError doesn't occur with $$$$ in markdown cell. What should I try to do?
UPD. OK, I've checked travis tests. Will fix this ASAP.

[pacahon]

@mpacer I guess it's better now, but still little bit odd. With $$$$ input we get "" while with $$ $$ we get the same string $$ $$. Maybe trim text inside math block?

[mpacer]

Why did you add the if text else "" bit? Was that strictly necessary?

I think that's introducing your comment:

With $$$$ input we get "" while with $$ $$ we get the same string $$ $$.

[mpacer]

Ok, new thought, i don't know why it wasn't succeeding in the first place when it ran into a $$$$ since the pattern it's matching is re.compile(r"^\$\$(.*?)\$\$|^\\\\\[(.*?)\\\\\]", re.DOTALL) which should return the empty string when we get the groups from that.

[mpacer]

Ok, thinking this through a bit more… at the point that we're returning the value, the $$$$ is actually being normalized by us, so at the least we can do if text else "$$$$". But I'm still a little confused as to why we even need to do this in the first place.

[mpacer]

Ok, did a couple of quick tests to make sure that I was following this, we need the "" as a fallback in the case where either $$$$ or \\[\\] fails. Right now, \\[\\] will work and if we swapped the order (e.g., m.group(2) or m.group(1) then $$$$ would work and \\[\\] would fail).

Since it was first triggered by the grammar match, we don't have to worry about returning an empty string in an inappropriate context.

So we can safely always return a fallback empty string, and then if we pass the empty string, it should correctly be interpreted as $$$$, which is what would be returned by return '$$%s$$' % self.escape_html(text) where text=="".

So, the question remains: why did your the test you introduced in 4f23786 fail — it should have been passed through without issue.

[mpacer]

@pacahon

Ok, just double checked it; the issue with your initial commit was that you represented the multiline string as a single line. My guess is you were editing the file as a JSON object in a text editor as opposed to creating a new notebook. For the purposes of our nbconvert test files, we are expecting to see a list of strings as the on-disk representation of notebook cells (which is why you had the error you ran into).

Anyway I created an example of how a notebook's cell should be formatted to pass the tests based on just opening and saving the notebook you modified in 4f23786.

mpacer@9afbb09

[pacahon]

@mpacer Sorry, misunderstood your comment here #726 (comment) Thought it was about block_math method, but looks like you told only about output_block_math. In this case I shouldn't edit block_math, of course. Anyway, since the problem related to renderer, isn't test_markdown is better place for this test?

[pacahon]

@mpacer Travis tests were failed with

The command "git clone --quiet --depth 1 https://github.com/minrk/travis-wheels travis-wheels" failed and exited with 128 during .

Could you please restart them? I didn't figure out how to do it.

[takluyver]

I've restarted the failing jobs (@mpacer is probably asleep at the moment).

[pacahon]

Thanks a lot!

[mpacer]

Apologies @pacahon, @takluyver was correct, I was asleep.

So… if our previous behaviour was to drop them, then we could continue to do that to be consistent with that. I'm not sure which is the more desired behaviour….

[pacahon]

@mpacer By "them" you mean "$$$$"?
I think it's ok to leave "$$$$" in rendered markdown as is. Later it will be rendered as empty line by mathjax and user should decide how to deal with it since he added empty math block for some purpose.

Btw previous behaviour was AttributeError if I'm not wrong. Or you mean something different?

[mpacer]

I meant the previous behaviour as in removing $$$$ since you needed to modify the test file to get it to pass. Somehow it wasn't triggering the error…

Anyway I'm going to merge this and say it's fixed, and we'll iterate on this again if it comes up.

[pacahon]

Thanks. Since I made a mess in commit history, I little bit curious is it possible to squash commits when merging a PR?

[takluyver]

It is, though in this case @mpacer did a conventional merge instead. Here's what the interface looks like:

[pacahon]

@takluyver Thanks for the clarification.