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
nbconvert: fixed latex characters not escaped properly in nbconvert #3951
Conversation
@ellisonbg this fixes the output for the |
Travis still failing on 2.6 and 2.7
|
This gives me three failures here (linux, python 2.7):
|
We've been looking at it on HipChat, and have a new, simpler regex-free approach that should work better. |
@@ -22,7 +22,7 @@ | |||
#Latex substitutions for escaping latex. | |||
LATEX_SUBS = ( | |||
(re.compile('\033\[[0-9;]+m'),''), # handle console escapes | |||
(re.compile(r'\\'), r'\\textbackslash'), | |||
(re.compile(r'\\'), r'{\\textbackslash}'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this change makes all tests pass for me. (only one test was failing with this PR: FAIL: escape_latex test
Mmh, now I'm seeing the last two of those three above also on master... Not good. It seems the issue is the latex tikz package, which I don't have on this box. Do you know if it's necessary?
I installed the ubuntu |
Ah @minrk, ok. So this one will get closed eventually, I take? |
@jdfreder maybe include the notebook Brian pointed you toward (or the relevant portion of it) into the test suite - not sure what portion of it is causing things to fail |
Yup, @jdfreder had to run, but I think he plans to get to it tonight or tomorrow morning. Or I can do it in the morning if he doesn't get to it. The new approach is very simple. |
(re.compile(r'"'), r"''"), | ||
(re.compile(r'\.\.\.+'), r'\\ldots'), | ||
) | ||
# Latex substitutions for escaping latex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still need to apply the first and last of these, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can apply the first without a regular expression. We already make that substitution in ansi.strip_ansi
, I'll just call that here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last also needed logic to be able to apply multicharacter replace
Added ... to \ldots escape Escape ansi like before
@minrk updated |
I made a PR against your branch last night - should have both fixes. |
Sorry I didn't catch that, I rushed over to here first thing in the morning |
I'll go take a peek |
This reverts commit 69adeb1. This allows me to merge min's code to give him credit.
@minrk I reverted my fix and merged yours so you could get credit 😁 |
You didn't have to do that - it was just putting the regex replacements back in that mattered. But thanks :) |
Any reason to do the ansi sub using a regex instead of the ansi.strip_ansi filter/function? |
return_text = text | ||
for pattern, replacement in LATEX_SUBS: | ||
return_text = pattern.sub(replacement, return_text) | ||
return return_text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove square brackets here
(re.compile(r'\^'), r'\^{}'), | ||
(re.compile(r'"'), r"''"), | ||
LATEX_RE_SUBS = ( | ||
(re.compile('\033\[[0-9;]+m'), ''), # handle console escapes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this one - it's redundant
for pattern, replacement in LATEX_SUBS: | ||
return_text = pattern.sub(replacement, return_text) | ||
return return_text | ||
text = ''.join([LATEX_SUBS.get(c, c) for c in text]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Square brackets not needed, generator comprehension will work by itself.
|
||
def escape_latex(text): | ||
""" | ||
Escape characters that may conflict with latex. | ||
|
||
Remove ansi codes and escape characters that may conflict with latex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this to reflect that it doesn't remove ansi codes
@@ -443,7 +443,7 @@ Note: For best display, use latex syntax highlighting. =)) | |||
((* macro custom_verbatim(text) -*)) | |||
\begin{alltt} | |||
((*- if resources.sphinx.centeroutput *))\begin{center} ((* endif -*)) | |||
((( text | wrap_text(wrap_size) ))) | |||
((( text | wrap_text(wrap_size) | escape_latex ))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be wrapped - it's already in a verbatim environment, wrapping messes that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added here as a safeguard. If it's not here, the output of a long sequence of character blows outside of the table and messes things up. Are you sure you want me to remove it? This won't allow users to output a long base64 string, a long byte string, etc... Latex isn't smart enough to break long words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I guess not. It's definitely doing the wrong thing, even with relatively short lines. Since it fixes a real issue, we can look at it more carefully at a later point. Shall I go ahead and merge this now, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This reverts commit 0b94505.
👍 |
nbconvert: fixed latex characters not escaped properly in nbconvert use simple dict lookup process instead of sequential regular expressions that confuse each other.
nbconvert: fixed latex characters not escaped properly in nbconvert use simple dict lookup process instead of sequential regular expressions that confuse each other.
No description provided.