Repository navigation
smarty: double sets of quotes not working at end #1514
Description
The following example renders incorrectly:
import markdown
text = '''
He replied, "She said 'Hello.'"
'''
html = markdown.markdown(text, extensions=['smarty'])
print(html)The output is:
<p>He replied, “She said ‘Hello.’“</p>
The “ at the end of this example should be ”.
The quote marks were replaced in this order:
- openingSingleQuotesRegex
- closingSingleQuotesRegex
- openingDoubleQuotesRegex
- remainingDoubleQuotesRegex
Unfortunately:
- closingDoubleQuotesRegex didn't match, because the
"is not followed by a space (it's the last character of the string, which is stripped prior to replacement). - closingDoubleQuotesRegex2 didn't match, because of the closing single quote replacement (i.e., the intermediate data in
SubstituteTextPattern.handleMatch()contains\u0003before the").
One possible way to fix this issue might be to change closingDoubleQuotesRegex to r'"(?=\s|$)' (i.e., double quote followed by whitespace or end of string). However that doesn't work in general, so a special case is likely needed.
Activity
facelessuser commentedon Mar 18, 2025
That doesn't seem like a bad solution.
What do you mean by this?
ChrisMayfield commentedon Mar 18, 2025
After posting the issue, I went back to what I was working on and realized my solution didn't work when wrapped in html tags, for example:
In this case, the closing double quote is not at the end of the string and is not followed by whitespace. So adding another regex similar to doubleQuoteSetsRe might be needed.
None of the other regex variables use
$so it's possible doing so for closingDoubleQuotesRegex might lead to unintended side effects.facelessuser commentedon Mar 18, 2025
I see, that makes sense. It may be that those tags are converted to placeholders creating a non-word boundary. I think it will need some investigation. I haven't taken a close look at the current pattern and its intentions. I'm sure there are some tricky cases where checking for space following the quote was important, I just don't know what all the cases are right now.
facelessuser commentedon Mar 18, 2025
It seems situations like this are also problematic (non-word boundaries):
My initial thought is it needs a non-word boundary check, but that might blow up some other cases. Definitely needs some further investigation to be sure.
Generally, IIRC, smarty quotes can be difficult to always get right the way we do them. Personally, I often turn off smarty quotes for these reasons, but I understand the desire to improve them and get them right (or at least better).
facelessuser commentedon Mar 18, 2025
I don't think a word boundary check would be correct, but maybe a not word check
(?=\s|\W|$). It seems to solve the the HTML case, the non-word case, and the end of buffer case without breaking any tests. I'm not sure if there are more cases I'm not considering though. I think @mitya57 was the original developer on this extension, so they would be the best to speak on this.mitya57 commentedon Mar 18, 2025
Hm, what exactly change did you try? I have just tried this:
but it does break one of the tests:
facelessuser commentedon Mar 18, 2025
@mitya57 You are correct. I ran
r'"(?=\s|\W)'and had not yet runr'"(?=\s|\W|$)'. The latter causes a test to fail.ChrisMayfield commentedon Mar 19, 2025
Thank you both for the suggestions. I can't think of a way to modify closingDoubleQuotesRegex without breaking the existing tests. And modifying closingDoubleQuotesRegex2 won't work either, because if a preceding single quote has already been replaced, closeClass will no longer match (due to
\u0003). This leads me to believe that both quote marks ('"or"') need to be matched at the same time. I'll submit a PR with this approach.Add special case for closing nested quotes