-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 typographic substitution in (pre|code|kbd|script) blocks when smartypants=true #1335
Fix typographic substitution in (pre|code|kbd|script) blocks when smartypants=true #1335
Conversation
lib/marked.js
Outdated
} else if (this.inRawBlock | ||
&& /^<\/pre>|^<\/code>|^<\/kbd>|^<\/script>/i.test(cap[0])) { | ||
this.inRawBlock = false; | ||
} |
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.
There's some duplication here: it'd be nice to just have a simple list of tags in which no substitution is to be performed -- ['pre', 'code', 'kbd', 'script']
-- 'cos that'd make it easy to add/remove tags; I did try that route but the regexes looked less "on the nose" than this version.
I also experimented with alternate names for the inRawBlock
field: they were all too darn long and made the code less readable: forex, isBlockInWhichNoSubstitutionIsToBePerformed
.
Happy to take a steer 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.
Could you change them to /^<(pre|code|kbd|script)/i
and /^<\/(pre|code|kbd|script)>/i
I think that makes it a little easier to see what the regex is looking for
smartypants: true | ||
description: SmartyPants does not modify characters within <pre>, <code>, <kbd>, or <script> tag blocks. | ||
spec: https://daringfireball.net/projects/smartypants/ | ||
--- |
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.
Just trying to be helpful by linking to the "spec" that explains why.
<pre>&</pre> | ||
<code>--foo</code> | ||
<kbd>---foo</kbd> | ||
<script>--foo</script> |
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 was pleasantly surprised by the form the test suite takes: adding a specification was so easy, good work Team Marked.
😍
lib/marked.js
Outdated
} else if (this.inRawBlock | ||
&& /^<\/pre>|^<\/code>|^<\/kbd>|^<\/script>/i.test(cap[0])) { | ||
this.inRawBlock = false; | ||
} |
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.
Could you change them to /^<(pre|code|kbd|script)/i
and /^<\/(pre|code|kbd|script)>/i
I think that makes it a little easier to see what the regex is looking for
Agreed, great suggestion 👍 I have done so. |
lib/marked.js
Outdated
@@ -730,6 +730,14 @@ InlineLexer.prototype.output = function(src) { | |||
} else if (this.inLink && /^<\/a>/i.test(cap[0])) { | |||
this.inLink = false; | |||
} | |||
if (!this.inRawBlock | |||
&& /^<(pre|code|kbd|script)/i.test(cap[0])) { |
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.
There should be a (\s|>)
at the end to prevent matching other tags that start with these 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.
Good one Tony; I've done so and expanded the tests to prove that.
Loving your attention to detail here, much appreciated 👍
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.
LGTM, thanks! 🎉
Marked version:
0.5.0
(latest)Markdown flavor: all
Description
Contributor
Committer
In most cases, this should be a different person than the contributor.