New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for an issue with empty math block #729

Merged
merged 5 commits into from Jan 13, 2018

Conversation

Projects
None yet
3 participants
@pacahon
Copy link
Contributor

pacahon commented Jan 9, 2018

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 9, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 9, 2018

@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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 9, 2018

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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 10, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 10, 2018

@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 added some commits Jan 10, 2018

@pacahon

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 10, 2018

@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

This comment has been minimized.

Copy link
Member

takluyver commented Jan 10, 2018

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

@pacahon

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 10, 2018

Thanks a lot!

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 10, 2018

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

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 10, 2018

@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

This comment has been minimized.

Copy link
Member

mpacer commented Jan 13, 2018

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.

@mpacer mpacer merged commit 627e431 into jupyter:master Jan 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pacahon

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 15, 2018

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

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 15, 2018

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

screenshot from 2018-01-15 11-44-25

@pacahon

This comment has been minimized.

Copy link
Contributor Author

pacahon commented Jan 15, 2018

@takluyver Thanks for the clarification.

@mpacer mpacer added this to the 5.4 milestone Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment