-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[Github] Improve formating of PR diffs in bot notifications #66118
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
Changes from all commits
72be28c
42bb7d5
b4b4e8f
d12147f
b645616
8c0b3bc
47ad7a0
1c7b435
96c4b47
b67d0fa
6a15e62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,16 @@ def _get_curent_team(team_name, teams) -> Optional[github.Team.Team]: | |
return None | ||
|
||
|
||
def escape_description(str): | ||
# https://github.com/github/markup/issues/1168#issuecomment-494946168 | ||
str = html.escape(str, False) | ||
# '@' followed by alphanum is a user name | ||
str = re.sub("@(?=\w+)", "@<!-- -->", str) | ||
# '#' followed by digits is considered an issue number | ||
str = re.sub("#(?=\d+\s)", "#<!-- -->", str) | ||
return str | ||
|
||
|
||
class IssueSubscriber: | ||
@property | ||
def team_name(self) -> str: | ||
|
@@ -67,12 +77,15 @@ def run(self) -> bool: | |
if team.slug == "issue-subscribers-good-first-issue": | ||
comment = "{}\n".format(beginner_comment) | ||
|
||
comment = ( | ||
f"@llvm/{team.slug}" | ||
+ "\n\n<details>\n" | ||
+ f"{self.issue.body}\n" | ||
+ "</details>" | ||
) | ||
body = escape_description(self.issue.body) | ||
|
||
comment = f""" | ||
@llvm/{team.slug} | ||
|
||
<details> | ||
{body} | ||
</details> | ||
""" | ||
|
||
self.issue.create_comment(comment) | ||
return True | ||
|
@@ -113,6 +126,11 @@ def run(self) -> bool: | |
print(f"couldn't find team named {self.team_name}") | ||
return False | ||
|
||
# GitHub limits comments to 65,536 characters, let's limit the diff | ||
# and the file list to 20kB each. | ||
STAT_LIMIT = 20 * 1024 | ||
DIFF_LIMIT = 20 * 1024 | ||
|
||
# Get statistics for each file | ||
diff_stats = f"{self.pr.changed_files} Files Affected:\n\n" | ||
for file in self.pr.get_files(): | ||
|
@@ -125,35 +143,41 @@ def run(self) -> bool: | |
if file.status == "renamed": | ||
print(f"(from {file.previous_filename})") | ||
diff_stats += "\n" | ||
diff_stats += "\n" | ||
if len(diff_stats) > STAT_LIMIT: | ||
break | ||
|
||
# Get the diff | ||
try: | ||
patch = html.escape(requests.get(self.pr.diff_url).text) | ||
patch = requests.get(self.pr.diff_url).text | ||
except: | ||
patch = "" | ||
diff_stats += "\n<pre>\n" + html.escape(patch) | ||
|
||
# GitHub limits comments to 65,536 characters, let's limit the diff to 20kB. | ||
DIFF_LIMIT = 20 * 1024 | ||
patch_link = f"Full diff: {self.pr.diff_url}\n" | ||
if len(patch) > DIFF_LIMIT: | ||
patch_link = f"\nPatch is {human_readable_size(len(patch))}, truncated to {human_readable_size(DIFF_LIMIT)} below, full version: {self.pr.diff_url}\n" | ||
diff_stats = diff_stats[0:DIFF_LIMIT] + "...\n<truncated>\n" | ||
diff_stats += "</pre>" | ||
patch = patch[0:DIFF_LIMIT] + "...\n[truncated]\n" | ||
team_mention = "@llvm/{}".format(team.slug) | ||
|
||
body = self.pr.body | ||
body = escape_description(self.pr.body) | ||
# Note: the comment is in markdown and the code below | ||
# is sensible to line break | ||
comment = f""" | ||
{self.COMMENT_TAG} | ||
{team_mention} | ||
|
||
<details> | ||
<summary>Changes</summary> | ||
|
||
{body} | ||
-- | ||
--- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change (three dashes) causes the last paragraph of the body to be rendered like a title, which looks awful, e.g.: the "After legalization is done" paragraph in this issue: #66567 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to add an extra new line before the dashes, I'll do that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
{patch_link} | ||
|
||
{diff_stats} | ||
|
||
``````````diff | ||
{patch} | ||
`````````` | ||
|
||
</details> | ||
""" | ||
|
||
|
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.
This isn't in the PR description, can you add it there? (with some context for the "why" of the change)