Skip to content

Commit 565eed2

Browse files
feat: Simplify diff hunk display and add comment filters
This commit refactors the `scripts/gha/get_pr_review_comments.py` script to simplify its output and add new filtering capabilities, based on your feedback. Key changes: - Diff Hunk Display: The complex contextual diff hunk display has been removed. The script now either shows the full diff hunk (if `--context-lines 0`) or the last N lines of the diff hunk (if `--context-lines N > 0`). The `print_contextual_diff_hunk` function was removed, and this logic is now inline. - Skip Outdated Comments: A new `--skip-outdated` flag allows you to exclude comments marked as [OUTDATED] from the output. - Line Number Display: For [OUTDATED] comments, the script now prefers `original_line` for the "Line in File Diff" field, falling back to `line`, then "N/A". - Metadata: Continues to display comment ID, reply ID, timestamp, status, user, file, URL, and body. These changes aim to make the script easier to maintain and its output more predictable, while still providing essential information and filtering options for reviewing PR comments.
1 parent 5948b96 commit 565eed2

File tree

1 file changed

+43
-55
lines changed

1 file changed

+43
-55
lines changed

scripts/gha/get_pr_review_comments.py

Lines changed: 43 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -28,44 +28,6 @@
2828
except ImportError:
2929
pass # firebase_github.py uses absl.logging.info, so this won't redirect.
3030

31-
32-
def print_contextual_diff_hunk(diff_hunk, comment_position, context_lines_count):
33-
if not diff_hunk or not diff_hunk.strip():
34-
print("(No diff hunk available or content is empty)")
35-
return
36-
37-
hunk_lines = diff_hunk.split('\n') # Note: Python's split('\n') is generally fine.
38-
39-
# Case 1: User explicitly wants the full hunk
40-
if context_lines_count == 0:
41-
print(diff_hunk)
42-
return
43-
44-
# Case 2: Contextual display is requested (context_lines_count > 0),
45-
# but comment is not on a specific line or position is invalid for contextual display.
46-
if comment_position is None or comment_position < 1 or comment_position > len(hunk_lines):
47-
print("(Comment is not on a specific line in the diff, or position is invalid; full hunk context suppressed by --context-lines setting)")
48-
# As an alternative to the above message, if the hunk is small, one might choose to print it.
49-
# However, sticking to the user's feedback of not wanting full hunks when context is specified:
50-
# print(diff_hunk) # This would be the old behavior for this case.
51-
return
52-
53-
# Case 3: Contextual display is possible and requested
54-
comment_line_index = comment_position - 1 # Convert to 0-indexed
55-
56-
start_index = max(0, comment_line_index - context_lines_count)
57-
end_index = min(len(hunk_lines), comment_line_index + context_lines_count + 1)
58-
59-
# The following line was identified as redundant and is removed:
60-
# start_index = min(start_index, comment_line_index if comment_line_index >=0 else 0)
61-
62-
for i in range(start_index, end_index):
63-
# Robust check, though start/end logic should prevent out-of-bounds
64-
if i >= 0 and i < len(hunk_lines):
65-
prefix = "> " if i == comment_line_index else " "
66-
print(f"{prefix}{hunk_lines[i]}")
67-
68-
6931
def main():
7032
default_owner = firebase_github.OWNER
7133
default_repo = firebase_github.REPO
@@ -101,15 +63,21 @@ def main():
10163
parser.add_argument(
10264
"--context-lines",
10365
type=int,
104-
default=10,
105-
help="Number of context lines around the commented line from the diff hunk. Use 0 for the full hunk. Default: 10."
66+
default=10, # Default to 10 lines, 0 means full hunk.
67+
help="Number of context lines from the diff hunk. Use 0 for the full hunk. "
68+
"If > 0, shows the last N lines of the hunk. Default: 10."
10669
)
10770
parser.add_argument(
10871
"--since",
10972
type=str,
11073
default=None,
11174
help="Only show comments created at or after this ISO 8601 timestamp (e.g., YYYY-MM-DDTHH:MM:SSZ)."
11275
)
76+
parser.add_argument(
77+
"--skip-outdated",
78+
action="store_true",
79+
help="If set, outdated comments will not be printed."
80+
)
11381

11482
args = parser.parse_args()
11583

@@ -127,11 +95,14 @@ def main():
12795
print(f"Fetching comments for PR #{args.pull_number} from {firebase_github.OWNER}/{firebase_github.REPO}...", file=sys.stderr)
12896
if args.since:
12997
print(f"Filtering comments created since: {args.since}", file=sys.stderr)
98+
if args.skip_outdated:
99+
print("Skipping outdated comments.", file=sys.stderr)
100+
130101

131102
comments = firebase_github.get_pull_request_review_comments(
132103
args.token,
133104
args.pull_number,
134-
since=args.since # Pass the 'since' argument
105+
since=args.since
135106
)
136107

137108
if not comments:
@@ -140,39 +111,56 @@ def main():
140111

141112
print("\n--- Review Comments ---")
142113
for comment in comments:
114+
# Determine outdated status and effective line for display
115+
is_outdated = comment.get("position") is None
116+
117+
if args.skip_outdated and is_outdated:
118+
continue
119+
120+
line_to_display = comment.get("original_line") if is_outdated else comment.get("line")
121+
# Ensure line_to_display has a fallback if None from both
122+
if line_to_display is None: line_to_display = "N/A"
123+
124+
143125
user = comment.get("user", {}).get("login", "Unknown user")
144126
path = comment.get("path", "N/A")
145-
file_line = comment.get("line", "N/A")
146-
hunk_position = comment.get("position") # This is the 1-indexed position in the hunk
147127

148128
body = comment.get("body", "").strip()
149-
diff_hunk = comment.get("diff_hunk") # Can be None or empty
150-
html_url = comment.get("html_url", "N/A")
129+
if not body: # Skip comments with no actual text body
130+
continue
151131

132+
diff_hunk = comment.get("diff_hunk")
133+
html_url = comment.get("html_url", "N/A")
152134
comment_id = comment.get("id")
153135
in_reply_to_id = comment.get("in_reply_to_id")
154136
created_at = comment.get("created_at")
155137

156-
if not body:
157-
continue
138+
status_text = "[OUTDATED]" if is_outdated else "[CURRENT]"
158139

140+
# Start printing comment details
159141
print(f"Comment by: {user} (ID: {comment_id}){f' (In Reply To: {in_reply_to_id})' if in_reply_to_id else ''}")
160142
if created_at:
161143
print(f"Timestamp: {created_at}")
162144

163-
if diff_hunk: # Only show status if it's a diff-related comment
164-
status_text = "[OUTDATED]" if hunk_position is None else "[CURRENT]"
165-
print(f"Status: {status_text}")
166-
145+
print(f"Status: {status_text}")
167146
print(f"File: {path}")
168-
print(f"Line in File Diff: {file_line}")
147+
print(f"Line in File Diff: {line_to_display}")
169148
print(f"URL: {html_url}")
170149

171150
print("--- Diff Hunk Context ---")
172-
if diff_hunk:
173-
print_contextual_diff_hunk(diff_hunk, hunk_position, args.context_lines)
151+
if diff_hunk and diff_hunk.strip():
152+
hunk_lines = diff_hunk.split('\n')
153+
if args.context_lines == 0: # User wants the full hunk
154+
print(diff_hunk)
155+
elif args.context_lines > 0: # User wants N lines of context (last N lines)
156+
lines_to_print_count = args.context_lines
157+
actual_lines_to_print = hunk_lines[-lines_to_print_count:]
158+
for line_content in actual_lines_to_print:
159+
print(line_content)
160+
# If context_lines < 0, argparse should ideally prevent this or it's handled by default type int.
161+
# No explicit handling here means it might behave unexpectedly or error if not positive/zero.
174162
else:
175-
print("(Comment not associated with a specific diff hunk)")
163+
print("(No diff hunk available for this comment)")
176164

177165
print("--- Comment ---")
178166
print(body)

0 commit comments

Comments
 (0)