rewrote the escape function to escape all markdown characters #242

yareeba · 2018-06-28T11:39:30Z

in order to fix issue #233

domchristie

Thanks for your contribution! 💫 I love the use of reduce!

I have made quite a few comments, so I hope this doesn't come across as ungrateful…

Overall, I like this approach. As I mentioned before, I think the library should escape aggressively to ensure that valid markdown is outputted. However, I don't think it is necessary to escape all markdown characters in any context (sorry, I should've mention this before). Escaping *, _, and ` in all contexts makes sense, but for the other characters, I think we can be a little less aggressive so that the output isn't overloaded with \.

As I said, I hope the comments aren't too overwhelming. Let me know what you think, and thanks again!

domchristie · 2018-06-29T05:25:54Z

src/turndown.js

@@ -6,6 +6,24 @@ import Node from './node'
 var reduce = Array.prototype.reduce
 var leadingNewLinesRegExp = /^\n*/
 var trailingNewLinesRegExp = /\n*$/
+const markdownReplacements = [


Perhaps var escapes = [

domchristie · 2018-06-29T05:37:07Z

src/turndown.js

+  [/-/g, '\\-'],
+  [/\+/g, '\\+'],
+  [/=/g, '\\='],
+  [/#/g, '\\#'],


# is only used for atx-style headings, so we can be more specific and reduce the number of escapes. Something like:

[/^(\W* {0,3})(#{1,6} )/gm, '$1\\$2']

Simplified this to

[/^(#{1,6})/g, '\\$1']

produces an escaped headline like this:

### headline = \### headline

Yes, a single backslash if sufficient: https://spec.commonmark.org/dingus/?text=%5C%23%23%23%20Not%20a%20heading%0A

domchristie · 2018-06-29T05:57:49Z

src/turndown.js

+  [/=/g, '\\='],
+  [/#/g, '\\#'],
+  [/`/g, '\\`'],
+  [/~/g, '\\~'],


~ can only be used for code blocks, so to reduce the number of escapes, something like:

[/^(\W* {0,3})~~~/gm, '$1\\~~~']

It can also be used for a strikethrough effect:

~~strikethrough~~ = ~~strikethrough~~

In this case shall we stick with the aggressive escaping for ~?

domchristie · 2018-06-29T06:01:30Z

src/turndown.js

+  [/#/g, '\\#'],
+  [/`/g, '\\`'],
+  [/~/g, '\\~'],
+  [/\|/g, '\\|'],


As pipes (|) are used for tables, this should probably be part of turndown-plugin-gfm, and so can be removed

domchristie · 2018-06-29T06:12:35Z

src/turndown.js

+  [/~/g, '\\~'],
+  [/\|/g, '\\|'],
+  [/\(/g, '\\('],
+  [/\)/g, '\\)'],


I don't think ( or ) need to be escaped if [ and ] are escaped, so we can remove these two lines

domchristie · 2018-06-29T11:51:35Z

test/index.html

-  <pre class="expected">You can use * for multiplication: 1.5 * 3 = 4.5</pre>
+<div class="case" data-name="escaping =">
+  <div class="input">A sentence containing =</div>
+  <pre class="expected">A sentence containing \=</pre>
 </div>


This test case can probably be removed

domchristie · 2018-06-29T11:54:56Z

test/index.html

  <div class="input">42 > 1</div>
-  <pre class="expected">42 > 1</pre>
+  <pre class="expected">42 \> 1</pre>


This test case can be reverted

domchristie · 2018-06-29T11:56:26Z

test/index.html

+<div class="case" data-name="escaping parentheses">
+  <div class="input">(A sentence containing)</div>
+  <pre class="expected">\(A sentence containing\)</pre>
+</div>


This test case can be removed

domchristie · 2018-06-29T11:56:34Z

test/index.html

+<div class="case" data-name="escaping |">
+  <div class="input">A sentence containing |</div>
+  <pre class="expected">A sentence containing \|</pre>
+</div>


This test case can be removed

domchristie · 2018-06-29T11:57:45Z

test/index.html

+<div class="case" data-name="escaping ~">
+  <div class="input">A sentence containing ~</div>
+  <pre class="expected">A sentence containing \~</pre>
+</div>


This should altered to only test a case where a ~ could be converted to markdown

yareeba · 2018-07-04T08:27:51Z

Hi @domchristie, thanks for the comments. Hopefully I will get a chance to look at this PR at some point this week and make changes. 👍

Paul-Ladyman · 2018-07-09T09:09:27Z

Hi @domchristie. I work with @ayusaf1992 and I've been looking through the PR. Your feedback seems to be mostly around not escaping certain characters so aggressively where they are only valid markdown in very specific circumstances and that makes sense to me. I've left a few replies here and there for further clarification but broadly speaking I think we'll try to find some time to make these changes this week :-).

Thanks for all your help!

…n valid markdown.

Paul-Ladyman · 2018-07-10T09:23:36Z

src/turndown.js

+  [/\\/g, '\\\\'],
+  [/\*/g, '\\*'],
+  [/-/g, '\\-'],
+  [/\+/g, '\\+'],


Paul-Ladyman · 2018-07-10T09:38:03Z

Hi @domchristie. In my recent commit I've made most of the changes discussed above. Outstanding issues are:

dealing with pipes in turndown-plugin-gfm
dealing with - given its used for bullets and hrs
how to escape ~ given they can be used for strikethrough.
dealing with _ (we've currently implemented it as &lowbar; for reasons described above)

We're keen to get this work merged as we have other priorities demanding our time. For that reason I'm thinking that the approach here to deal with |, - and ~ could stay as it is and be improved as part of a separate bit of work that we could perhaps leave to yourself. What do you think?

That just leaves _ which I'm a little less sure of how to handle. Any thoughts given the comment above?

Cheers.

domchristie · 2018-07-11T09:28:29Z

Hi @Paul-Ladyman, thanks so much for your work on this. Sorry it has taken a couple of days to get back to you (I have been busy working as well as enjoying the rather nice weather!) Please see my comments below:

dealing with pipes in turndown-plugin-gfm
how to escape ~ given they can be used for strikethrough.

By default, I don't think that Turndown should escape non-commonmark characters, so it shouldn't escape pipes. Tildes are an interesting case due to the way they are used in other markdown flavours. I think the ultimate solution to this would be to add an "escape" API (disableEscape('~'), addEscape('~' , …)), but this is definitely one for another time. In the meantime I think we should go with the less aggressive escape for tildes. escape will be documented as public API, and as a recommended place to customise escaping (#244 (comment)). Sorry this isn't ideal for your use-case.

dealing with - given its used for bullets and hrs
Do you think as a first pass it would be reasonable to escape - aggressively and work on more sophisticated escaping as part of a future task?

Yes, I think this is fine for now. I can work on implementing function replacements later.

dealing with _ (we've currently implemented it as &lowbar; for reasons described above)

I'm not keen on &lowbar; or HTML entities, because underscores are reasonably common in writing, and Turndown is often used to obtain a clean plain text version of an HTML document, so I'd likely get a lot of issues raised as a result :/ With that in mind, I think we should stick with plain underscore escaping (sorry!). I'd be interested to know why plain underscore escaping does not work for you, and if it's a common issue then we can reconsider implementing &lowbar;.

As I mentioned, I think we can improve the escaping feature, but in the meantime, you may want to override the escape method with something like:

var oldEscape = TurndownService.prototype.escape

TurndownService.prototype.escape = function (string) {
  var escapes = [
    [/[^\\]~/g, '\\~'], // only escape ~ that have not already been escaped
    [/\|/g, '\\|']
  ]
  string = string.replace(/_/g, '&lowbar;')
  string = oldEscape(string)
  
  return escapes.reduce(function (accumulator, escape) {
    return accumulator.replace(escape[0], escape[1])
  }, string)
}

- Don't escape | here as its not commonmark - Use regular escaping for _ rather than HTML entities

Paul-Ladyman · 2018-07-18T08:49:10Z

Hi @domchristie,

Thanks again for your feedback!

Thinking more about pipes the gfm table syntax is so specific that I don't think we (my colleagues and I) should be immediately worried about our users recreating it. And I think its a good idea to keep the gfm stuff to the plugin so I've removed it from this PR. Updating the plugin may not be something we do ourselves in the short term however.

I didn't realise that the use of tilde for strikethrough was part of gfm. I've updated the regex to [/~~~/g, '\\~~~'] since otherwise it was having trouble picking up the opening and closing block tags. This should still result in less escaping.

I've also changed the _ escaping to normal backslashes. We weren't really expecting this to be merged but thought it was worth the conversation. I like your suggestion of how to add extra escaping on top of Turndown's so I think we'll do something like this whilst we try to get to the bottom of the issue.

Think that about covers it :-).

Cheers!

domchristie · 2018-07-18T19:32:37Z

Hi @Paul-Ladyman

Thanks again for your feedback!

Thank you! I really appreciate the time you and your colleagues (@ayusaf1992 & @olih) have put into this. Out of interest, are you using it for anything interesting? Let me know if you are able to do so :)

Thinking more about pipes the gfm table syntax is so specific that I don't think we (my colleagues and I) should be immediately worried about our users recreating it. And I think its a good idea to keep the gfm stuff to the plugin so I've removed it from this PR. Updating the plugin may not be something we do ourselves in the short term however.

👍

I didn't realise that the use of tilde for strikethrough was part of gfm. I've updated the regex to [/~~~/g, '\~~~'] since otherwise it was having trouble picking up the opening and closing block tags. This should still result in less escaping.

👍

I've also changed the _ escaping to normal backslashes. We weren't really expecting this to be merged but thought it was worth the conversation. I like your suggestion of how to add extra escaping on top of Turndown's so I think we'll do something like this whilst we try to get to the bottom of the issue.

👍

The changes will be in the next version which will be released soon. Thanks again!

domchristie · 2018-08-21T08:15:38Z

Released in v5.0.0

Paul-Ladyman · 2018-08-22T13:28:29Z

Hi @domchristie,

Nice to see this got released.

Sorry I didn't answer your question before. @ayusaf1992, @olih and I work for the BBC. We're implementing a new internal article editor that journalists working in for example BBC Sport or BBC News will use to write and publish articles to their respective sites. The editor uses SlateJS and we use Markdown to represent Slate's rich text "marks" in a more common format.

We're actually thinking of moving away from Markdown as we have some requirements that don't seem to be a standard part of any spec. For example representing the difference between text written in a left-to-right language and text in a right-to-left language.

In any case we were happy to help out with this bug fix and pleased it ended up getting released :-).

Cheers,
Paul

rewrote the escape function to escape all markdown characters

0140cd5

domchristie requested changes Jun 29, 2018

View reviewed changes

domchristie mentioned this pull request Jun 29, 2018

Add the option to disable escaping markdown characters #244

Closed

Changes after code review. Escape certain characters only when used i…

3c1faec

…n valid markdown.

Paul-Ladyman reviewed Jul 10, 2018

View reviewed changes

Paul added 2 commits July 18, 2018 09:17

Changes after code review

207052d

- Don't escape | here as its not commonmark - Use regular escaping for _ rather than HTML entities

Changes after code review. Only escape ~ if used as code block.

a26c454

domchristie approved these changes Jul 18, 2018

View reviewed changes

domchristie merged commit aad75e1 into mixmark-io:master Jul 18, 2018

This was referenced Aug 21, 2018

escaping double asterisks and underscores #233

Closed

escaping problem #220

Closed

domchristie mentioned this pull request Jan 1, 2019

Special character escaping is too aggressive #267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrote the escape function to escape all markdown characters #242

rewrote the escape function to escape all markdown characters #242

yareeba commented Jun 28, 2018 •

edited

Loading

domchristie left a comment •

edited

Loading

domchristie Jun 29, 2018

Paul-Ladyman Jul 10, 2018

domchristie Jun 29, 2018

Paul-Ladyman Jul 10, 2018

domchristie Jul 11, 2018

domchristie Jun 29, 2018

Paul-Ladyman Jul 9, 2018

domchristie Jun 29, 2018

domchristie Jun 29, 2018

Paul-Ladyman Jul 10, 2018

domchristie Jun 29, 2018

domchristie Jun 29, 2018

domchristie Jun 29, 2018

domchristie Jun 29, 2018

domchristie Jun 29, 2018

yareeba commented Jul 4, 2018

Paul-Ladyman commented Jul 9, 2018

Paul-Ladyman Jul 10, 2018

Paul-Ladyman commented Jul 10, 2018

domchristie commented Jul 11, 2018

Paul-Ladyman commented Jul 18, 2018

domchristie commented Jul 18, 2018

domchristie commented Aug 21, 2018

Paul-Ladyman commented Aug 22, 2018

rewrote the escape function to escape all markdown characters #242

rewrote the escape function to escape all markdown characters #242

Conversation

yareeba commented Jun 28, 2018 • edited Loading

domchristie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yareeba commented Jul 4, 2018

Paul-Ladyman commented Jul 9, 2018

Choose a reason for hiding this comment

Paul-Ladyman commented Jul 10, 2018

domchristie commented Jul 11, 2018

Paul-Ladyman commented Jul 18, 2018

domchristie commented Jul 18, 2018

domchristie commented Aug 21, 2018

Paul-Ladyman commented Aug 22, 2018

yareeba commented Jun 28, 2018 •

edited

Loading

domchristie left a comment •

edited

Loading