Skip to content
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.

Sign up for GitHub

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

Adding code tags around any nwo#number text string #5646

Merged
merged 8 commits into from
Sep 7, 2022
Merged

Adding code tags around any nwo#number text string #5646

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3605fbf
Adding a space in link text and any nwo#number text string
pavera Sep 7, 2022
b07430e
Use `<code/>` tags instead of spaces
pavera Sep 7, 2022
0f6b6c7
check if the parent is a link before wrapping in code block
pavera Sep 7, 2022
7c99a8a
Cleanup the complexity a bit.
pavera Sep 7, 2022
3f8fe0c
lint cleanup
pavera Sep 7, 2022
730c4c1
Removing trailing whitespaces
honeyankit Sep 7, 2022
6fa21ee
Merge branch 'main' into pavera/add-spaces-to-nwo-refs
pavera Sep 7, 2022
640186e
adding a comment about the regex
pavera Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"name": "Debug Tests",
"type": "Ruby",
"request": "launch",
"program": "${workspaceRoot}/${input:ecosystem}/.bundle/bin/rspec",
"program": "${workspaceRoot}/omnibus/.bundle/bin/rspec",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was necessitated by #5529

"cwd": "${workspaceRoot}/${input:ecosystem}",
"useBundler": true,
"args": ["${input:test_path}"],
Expand Down Expand Up @@ -106,6 +106,7 @@
"options": [
"bundler",
"cargo",
"common",
"composer",
"docker",
"elm",
Expand Down
27 changes: 27 additions & 0 deletions common/lib/dependabot/pull_request_creator/message_builder/link_and_mention_sanitizer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@Nishnha Nishnha Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this regex? Specifically [^/\s#]+

Copy link
Contributor Author

@pavera pavera Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (#)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavera : could you put the regex explanation in the comment as well. It will be easier for future debugging.

MENTION_REGEX = %r{(?<![A-Za-z0-9`~])@#{GITHUB_USERNAME}/?}.freeze
# regex to match a team mention on github
TEAM_MENTION_REGEX = %r{(?<![A-Za-z0-9`~])@(?<org>#{GITHUB_USERNAME})/(?<team>#{GITHUB_USERNAME})/?}.freeze
Expand All @@ -40,6 +41,7 @@ def sanitize_links_and_mentions(text:, unsafe: false)
sanitize_team_mentions(doc)
sanitize_mentions(doc)
sanitize_links(doc)
sanitize_nwo_text(doc)

mode = unsafe ? :UNSAFE : :DEFAULT
doc.to_html(([mode] + COMMONMARKER_OPTIONS), COMMONMARKER_EXTENSIONS)
Expand Down Expand Up @@ -109,6 +111,25 @@ def sanitize_links(doc)
end
end

def sanitize_nwo_text(doc)
doc.walk do |node|
if node.type == :text &&
node.string_content.match?(GITHUB_NWO_REGEX) &&
!parent_node_link?(node)
replace_nwo_node(node)
end
end
end

def replace_nwo_node(node)
match = node.string_content.match(GITHUB_NWO_REGEX)
repo = match.named_captures.fetch("repo")
number = match.named_captures.fetch("number")
new_node = build_nwo_text_node("#{repo}##{number}")
node.insert_before(new_node)
node.delete
end

def replace_github_host(text)
text.gsub(
/(www\.)?github.com/, github_redirection_service || "github.com"
Expand Down Expand Up @@ -171,6 +192,12 @@ def build_mention_link_text_nodes(text)
[code_node]
end

def build_nwo_text_node(text)
code_node = CommonMarker::Node.new(:code)
code_node.string_content = text
code_node
end

def create_link_node(url, text)
link_node = CommonMarker::Node.new(:link)
code_node = CommonMarker::Node.new(:code)
Expand Down
11 changes: 11 additions & 0 deletions ...n/spec/dependabot/pull_request_creator/message_builder/link_and_mention_sanitizer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,17 @@
end
end

context "with a GitHub NWO and PR number" do
let(:text) do
"dsp-testing/dependabot-ts-definitely-typed#25"
end
it do
is_expected.to eq(
"<p><code>dsp-testing/dependabot-ts-definitely-typed#25</code></p>\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to include the newline at the end of the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the call here renders the doc and adds the newline, so the test check has to expect it as far as I know.

)
end
end

context "with a GitHub link in rdoc" do
let(:text) do
"{Issue 111}[https://github.com/dependabot/dependabot-core/issues/111]"
Expand Down