Adding code tags around any nwo#number text string #5646
Conversation
|
I've now limited this to only find text strings and not link text. I am concerned as copy/pasting a link such as: https://github.com/dependabot/dependabot-core/blob/main/common/spec/dependabot/pull_request_creator/message_builder/link_and_mention_sanitizer_spec.rb#L278 results in: \nI feel like we should probably be escaping the link text as well. |
pavera
changed the title
| @@ -14,6 +14,7 @@ class LinkAndMentionSanitizer | |||
| github\.com/(?<repo>#{GITHUB_USERNAME}/[^/\s]+)/ | |||
| (?:issue|pull)s?/(?<number>\d+) | |||
| }x.freeze | |||
| GITHUB_NWO_REGEX = %r{(?<repo>#{GITHUB_USERNAME}/[^/\s#]+)#(?<number>\d+)}.freeze | |||
There was a problem hiding this comment.
Could you explain this regex? Specifically [^/\s#]+
There was a problem hiding this comment.
I cribbed it together from the above regex that grabs the repo and number from a full github pr or issue link. Given a string like myorg/myrepo#123 the regex will match myorg based on the GITHUB_USERNAME regex, then will match nonwhitespace and non # chars after a / as the repo name, then grab the digits after the # as the issue or PR number. Specifically [^/\s#]+ says: match 1 or many characters (+) as long as they don't match (^) the following: forward slash (/), whitespace (\s), or hash symbol (#)
There was a problem hiding this comment.
@pavera : could you put the regex explanation in the comment as well. It will be easier for future debugging.
| end | ||
| it do | ||
| is_expected.to eq( | ||
| "<p><code>dsp-testing/dependabot-ts-definitely-typed#25</code></p>\n" |
There was a problem hiding this comment.
Do we need to include the newline at the end of the line?
There was a problem hiding this comment.
the call here renders the doc and adds the newline, so the test check has to expect it as far as I know.
|
| @@ -20,7 +20,7 @@ | |||
| "name": "Debug Tests", | |||
| "type": "Ruby", | |||
| "request": "launch", | |||
| "program": "${workspaceRoot}/${input:ecosystem}/.bundle/bin/rspec", | |||
| "program": "${workspaceRoot}/omnibus/.bundle/bin/rspec", | |||
There was a problem hiding this comment.
What was the rationale for this? I don't see it mentioned in the commit messages / PR description, so was surprised to see it... did I just miss it somehow?
There was a problem hiding this comment.
This change was necessitated by #5529
Attempting to improve our link sanitation to prevent
<org>/<repo>#<pr number>text strings from being hydrated into full github.com links by PR creation machinery.I found that a number of our tests produce redirect links, however the link text itself is in the format above, and adding those links to a PR comment or description results in a valid github.com link and creates a mention on the targeted PR timeline.
This should also break up any text string in the above format to prevent flood of mentions experienced in the below issue:
Closes #5639
I also added common tests to the test debug dropdown in vscode and fixed a path issue with the vscode debugger.
This is still a WIP as there is 1 failing test which I need to fix, and the approach may be overly heavy handed. I'm not sure my tests which resulted in lots of valid links were valid, so feedback there would be good.