Added support for italics to bugdown #2107
Conversation
There was a problem hiding this comment.
Cool, thanks for working on this @TigorC! I posted several comments. See http://learnhtmlwithsong.com/blog/07-1-use-italics-use-emphasis/ for why <em> is preferred over <i>.
| @@ -978,6 +978,11 @@ def extendMarkdown(self, md, md_globals): | |||
| markdown.inlinepatterns.SimpleTagPattern(r'(\*\*)([^\n]+?)\2', 'strong'), | |||
| '>not_strong') | |||
|
|
|||
| md.inlinePatterns.add( | |||
| 'italic', | |||
| markdown.inlinepatterns.SimpleTagPattern(r'(\*)([\w|\ |\t]+)\*', 'i'), | |||
There was a problem hiding this comment.
Can you add a comment explaining the intended behavior and why this regular expression is correct?
| out += this.renderer.italic(this.output(cap[1])); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Just a check: Would using the em logic in the next block work instead? I think technically in markdown it's supposed to do <em>, so maybe we don't need to add this code but just should be adjusting the above.
| converted = bugdown.convert(msg) | ||
| self.assertEqual( | ||
| converted, | ||
| "<ul>\n<li>item1</li>\n<li>item2</li>\n</ul>") |
There was a problem hiding this comment.
Why not just put these tests in bugdown-data.json so we can test with both markdown processors?
| @@ -978,6 +978,11 @@ def extendMarkdown(self, md, md_globals): | |||
| markdown.inlinepatterns.SimpleTagPattern(r'(\*\*)([^\n]+?)\2', 'strong'), | |||
| '>not_strong') | |||
|
|
|||
| md.inlinePatterns.add( | |||
There was a problem hiding this comment.
Should this be called emphasis instead?
TigorC
force-pushed
the
italic_markdown
branch
from
efee6a8
to
16bc927
Compare
| disable_markdown_regex(marked.InlineLexer.rules.zulip, 'em'); | ||
|
|
||
| // Disable _emphasis_ | ||
| marked.InlineLexer.rules.zulip.em = /^\*((?:\*\*|[\w|\ |\t])+?)\*(?!\*)/; |
There was a problem hiding this comment.
can you explain how this change works (maybe in an extended comment? I don't think it's at all clear from this why we're changing the regular expression in this way.
TigorC
force-pushed
the
italic_markdown
branch
from
16bc927
to
08df703
Compare
|
Can you rebase? I just merged the strikethrough support. |
| // Disable special characters for the emphasis syntax | ||
| // Inside ** allowed only: word characters/spaces/tabulate | ||
| // it need for things like "const char *x = (char *)y" | ||
| marked.InlineLexer.rules.zulip.em = /^\*((?:\*\*|[\w|\ |\t])+?)\*(?!\*)/; |
There was a problem hiding this comment.
Why is this regular expression different from the one in bugdown/init.py? It seems like we want to use the same regular expressions in the two places.
Also, why are you limiting the characters inside in any way other than forbidding * and \n and controlling how we handle whitespace; what I had in mind was just require that the character after the first * is a non-whitespace character and the character before the second star is a non-whitespace character. So *foo bar* would be italics but * foo bar* and similar things would not be; that covers the const char *x = (char *)y issue fine (since the second * has a space in it).
TigorC
force-pushed
the
italic_markdown
branch
from
08df703
to
b88126e
Compare
|
Merged (after squashing, since the first commit along fails the frontend tests), thanks @TigorC! |
This closes #1103