[MarkdownFixer] Do not escape username with underscores in code or codeblocks #5785

JuanitoFatas · 2020-01-28T05:35:05Z

What type of PR is this? (check all applicable)

Refactor
Feature
Bug Fix
Optimization
Documentation Update

Tests are passing

bundle exec rspec spec/labor/markdown_fixer_spec.rb spec/requests/api/v0/articles_spec.rb spec/services/articles/creator_spec.rb spec/services/mentions/create_all_spec.rb

I‘ve verified it works on development
starts the server at local, sign in, new post, copy the following to preview and see if things work as expected. You should not see \ in code and codeblock, and underscored username renders correctly.
Code for testing at development
```
@_dev_

```
@_dev_
```

`@_dev_`

@_dev_
```

Description

The underscored username got escaped in code and code blocks. We probably do not want that. The fix is only to escape underscored username if they are not in code or code blocks.

Other possible approaches:

Write regexp that only pull out underscored usernames not in code or code blocks
Turn markdown into HTML and use Nokogiri to work on text of ancestors not pre or code

Related Tickets & Documents

Fixes #5739.

Screenshots

Current on dev (2019.01.28, `26e4412`)

After this Pull Request (tested against `432885f`)

Added to documentation?

docs.dev.to
readme
no documentation needed
code comments added

[optional] What gif best describes this PR or how it makes you feel?

claassistantio · 2020-01-28T05:35:47Z

All committers have signed the CLA.

Zhao-Andy

This looks good to me, thanks for the fix!

Zhao-Andy · 2020-02-03T19:19:54Z

I assume this PR is good because all tests are passing now, but @JuanitoFatas let me know if I'm mistaken. :)

JuanitoFatas · 2020-02-04T00:42:43Z

@Zhao-Andy

I assume this PR is good because all tests are passing now, but @JuanitoFatas let me know if I'm mistaken. :)

Rebased with master and verified it works ✅

and tests passed 🎉

rhymes

Thank you @JuanitoFatas, markdown parsing bugs are always complicated to fix, your solution seems very clever!

I left notes about adding comments to the new code as node traversal and regexp aren't usually super intuitive for future new readers.

ps. don't forget to prefix your branches with your name in the future, you can checkout the contributing guide for details.

app/labor/markdown_fixer.rb

JuanitoFatas · 2020-02-09T13:51:22Z

@rhymes

Thanks for the review. I‘ve added comments and changed the implementation to a less clever one 😉 . I also pulled the node traverser into a separate file, hope that make it easier to read. Verified this works and updated the screenshot in the Pull Request accordingly 🎉

citizen428 · 2020-02-10T03:45:14Z

app/labor/markdown_fixer.rb

@@ -47,11 +47,29 @@ def split_tags(markdown)
    end

    def underscores_in_usernames(markdown)
-      markdown.gsub(/(@)(_\w+)(_)/, '\1\\\\\2\\\\\3')
+      return markdown unless markdown.include?("@")


This guard clause has to go through all of markdown to determine whether or not to return, so I don't think it saves us much work. If we skip this check we go through the document once, if we keep it we go through the document once + the percentage of text preceding the username.

There are number of tests that pass "", front matters, or text that do not have username at all (without @) to here. These make sense in real world. Hence checking for @ here to avoid doing work for irrelevant cases and all tests are happy.

I understand the reason for the check, my only slight concern was this:

Case 1: markdowndoesn't contain an @: we're still going to compare every character in the document to @ for the include? check. I'd guess a majority of articles won't have usernames in them.

Case 2. markdown contains an @: we read markdown once until we find the @, then traverse again.

Maybe the guard clause should actually make use of the regular expression you defined, something like

return markdown unless markdown =~ USERNAME_WITH_UNDERSCORE_REGEXP

as that's actually what we care about, since the @ sign does not necessarily indicate a username (in some languages it's used for array concatenation for example).

Hey checking against the regexp makes sense. I‘ve updated the code.

app/labor/markdown_fixer.rb

app/labor/markdown_traverser.rb

JuanitoFatas

🏓

JuanitoFatas · 2020-02-10T13:07:52Z

app/labor/markdown_fixer.rb

@@ -47,11 +47,29 @@ def split_tags(markdown)
    end

    def underscores_in_usernames(markdown)
-      markdown.gsub(/(@)(_\w+)(_)/, '\1\\\\\2\\\\\3')
+      return markdown unless markdown.include?("@")


There are number of tests that pass "", front matters, or text that do not have username at all (without @) to here. These make sense in real world. Hence checking for @ here to avoid doing work for irrelevant cases and all tests are happy.

app/labor/markdown_fixer.rb

app/labor/markdown_traverser.rb

citizen428

Left a bunch more comments, but don't let any of them stop you.

It only runs when markdown has underscored usernames. Skip escaping underscore when content is in code or codeblock. It works by going through all lines of a markdown content. Find underscored username. Escape if we are not in the codeblock. The (?<!) is negative lookbehind which rules out the case when a underscored username is present in code, e.g., `@_dev_` will not be escaped.

citizen428

This looks good now, thanks for updating the guard clause.

maestromac

This is a very awesome solution. LGTM!

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020

JuanitoFatas force-pushed the markdown_fixer/not-escape-for-code-or-codeblocks branch from d486f87 to 6591cb7 Compare January 28, 2020 08:37

rhymes changed the title ~~[MarkdownFixer] Do not escape underscores in code or codeblocks~~ [WIP] [MarkdownFixer] Do not escape underscores in code or codeblocks Jan 28, 2020

pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020

JuanitoFatas force-pushed the markdown_fixer/not-escape-for-code-or-codeblocks branch from 6591cb7 to 5315367 Compare January 28, 2020 13:13

JuanitoFatas changed the title ~~[WIP] [MarkdownFixer] Do not escape underscores in code or codeblocks~~ [MarkdownFixer] Do not escape underscores in code or codeblocks Jan 28, 2020

JuanitoFatas requested a review from rhymes January 28, 2020 13:25

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020

JuanitoFatas changed the title ~~[MarkdownFixer] Do not escape underscores in code or codeblocks~~ [WIP] [MarkdownFixer] Do not escape username with underscores in code or codeblocks Jan 28, 2020

pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020

JuanitoFatas changed the title ~~[WIP] [MarkdownFixer] Do not escape username with underscores in code or codeblocks~~ [MarkdownFixer] Do not escape username with underscores in code or codeblocks Jan 29, 2020

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 29, 2020

rhymes requested review from maestromac and Zhao-Andy and removed request for rhymes January 30, 2020 13:20

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

Zhao-Andy previously approved these changes Feb 3, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 3, 2020

JuanitoFatas dismissed Zhao-Andy’s stale review via 477f152 February 4, 2020 00:30

JuanitoFatas force-pushed the markdown_fixer/not-escape-for-code-or-codeblocks branch from 5315367 to 477f152 Compare February 4, 2020 00:30

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 4, 2020

Zhao-Andy previously approved these changes Feb 4, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 4, 2020

mstruve requested a review from rhymes February 6, 2020 18:50

mstruve requested a review from citizen428 February 6, 2020 18:50

rhymes reviewed Feb 7, 2020

View reviewed changes

app/labor/markdown_fixer.rb Outdated Show resolved Hide resolved

app/labor/markdown_fixer.rb Show resolved Hide resolved

JuanitoFatas dismissed Zhao-Andy’s stale review via 432885f February 9, 2020 13:35

JuanitoFatas force-pushed the markdown_fixer/not-escape-for-code-or-codeblocks branch from 477f152 to 432885f Compare February 9, 2020 13:35

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 9, 2020

citizen428 reviewed Feb 10, 2020

View reviewed changes

JuanitoFatas commented Feb 10, 2020

View reviewed changes

JuanitoFatas requested a review from citizen428 February 10, 2020 13:12

citizen428 reviewed Feb 11, 2020

View reviewed changes

JuanitoFatas force-pushed the markdown_fixer/not-escape-for-code-or-codeblocks branch from 0e92d06 to 2ac92e8 Compare February 13, 2020 14:32

rhymes requested a review from citizen428 February 13, 2020 14:53

citizen428 approved these changes Feb 14, 2020

View reviewed changes

rhymes requested a review from Zhao-Andy February 14, 2020 07:48

maestromac approved these changes Feb 25, 2020

View reviewed changes

maestromac merged commit cc09ed7 into forem:master Feb 25, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Feb 25, 2020

citizen428 mentioned this pull request May 25, 2020

triple backticks preceded by other characters should not mark the start/end of a code block #8044

Open

JuanitoFatas deleted the markdown_fixer/not-escape-for-code-or-codeblocks branch September 16, 2020 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MarkdownFixer] Do not escape username with underscores in code or codeblocks #5785

[MarkdownFixer] Do not escape username with underscores in code or codeblocks #5785

JuanitoFatas commented Jan 28, 2020 •

edited

Loading

claassistantio commented Jan 28, 2020 •

edited

Loading

Zhao-Andy left a comment

Zhao-Andy commented Feb 3, 2020

JuanitoFatas commented Feb 4, 2020

rhymes left a comment

JuanitoFatas commented Feb 9, 2020

citizen428 Feb 10, 2020

JuanitoFatas Feb 10, 2020

citizen428 Feb 11, 2020 •

edited

Loading

JuanitoFatas Feb 13, 2020

JuanitoFatas left a comment

JuanitoFatas Feb 10, 2020

citizen428 left a comment

citizen428 left a comment

maestromac left a comment

[MarkdownFixer] Do not escape username with underscores in code or codeblocks #5785

[MarkdownFixer] Do not escape username with underscores in code or codeblocks #5785

Conversation

JuanitoFatas commented Jan 28, 2020 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Screenshots

Current on dev (2019.01.28, 26e4412)

After this Pull Request (tested against 432885f)

Added to documentation?

[optional] What gif best describes this PR or how it makes you feel?

claassistantio commented Jan 28, 2020 • edited Loading

Zhao-Andy left a comment

Choose a reason for hiding this comment

Zhao-Andy commented Feb 3, 2020

JuanitoFatas commented Feb 4, 2020

rhymes left a comment

Choose a reason for hiding this comment

JuanitoFatas commented Feb 9, 2020

citizen428 Feb 10, 2020

Choose a reason for hiding this comment

JuanitoFatas Feb 10, 2020

Choose a reason for hiding this comment

citizen428 Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

JuanitoFatas Feb 13, 2020

Choose a reason for hiding this comment

JuanitoFatas left a comment

Choose a reason for hiding this comment

JuanitoFatas Feb 10, 2020

Choose a reason for hiding this comment

citizen428 left a comment

Choose a reason for hiding this comment

citizen428 left a comment

Choose a reason for hiding this comment

maestromac left a comment

Choose a reason for hiding this comment

JuanitoFatas commented Jan 28, 2020 •

edited

Loading

Current on dev (2019.01.28, `26e4412`)

After this Pull Request (tested against `432885f`)

claassistantio commented Jan 28, 2020 •

edited

Loading

citizen428 Feb 11, 2020 •

edited

Loading