Link to comments as they are in files. (#133)

src/scripts/pr_comments.py

@@ -4,6 +4,7 @@
 
 import argparse
 import datetime
+import hashlib
 import os
 import sys
 import textwrap
@@ -57,8 +58,10 @@ _QUERY_REVIEW_THREADS = """
               body
               createdAt
               originalPosition
+              originalCommit {
+                abbreviatedOid
+              }
               path
-              url
             }
           }
           isResolved
@@ -133,13 +136,44 @@ class _Comment(object):
 class _Thread(object):
     """A review thread on a line of code."""
 
-    def __init__(self, thread):
+    def __init__(self, parsed_args, thread):
         self.is_resolved = thread["isResolved"]
 
         comments = thread["comments"]["nodes"]
-        self.line = comments[0]["originalPosition"]
-        self.path = comments[0]["path"]
-        self.url = comments[0]["url"]
+        first_comment = comments[0]
+        self.line = first_comment["originalPosition"]
+        self.path = first_comment["path"]
+
+        # Link to the comment in the commit if possible; GitHub features work
+        # better there than in the conversation view. The diff_url allows
+        # viewing changes since the comment, although the comment won't be
+        # visible there.
+        if "originalCommit" in first_comment:
+            template = (
+                "https://github.com/carbon-language/%(repo)s/pull/%(pr_num)s/"
+                "files/%(oid)s%(head)s#diff-%(path_md5)s%(line_side)s%(line)s"
+            )
+            # GitHub uses an md5 of the file's path for the link.
+            path_md5 = hashlib.md5()
+            path_md5.update(bytearray(self.path, "utf-8"))
+            format_dict = {
+                "head": "",
+                "line_side": "R",
+                "line": self.line,
+                "oid": first_comment["originalCommit"]["abbreviatedOid"],
+                "path_md5": path_md5.hexdigest(),
+                "pr_num": parsed_args.pr_num,
+                "repo": parsed_args.repo,
+            }
+            self.comment_url = template % format_dict
+            format_dict["head"] = "..HEAD"
+            format_dict["line_side"] = "L"
+            self.diff_url = template % format_dict
+            self.conversation_url = None
+        else:
+            self.conversation_url = first_comment["url"]
+            self.comment_url = None
+            self.diff_url = None
 
         self.comments = [
             _Comment.from_raw_comment(comment)
@@ -163,15 +197,15 @@ class _Thread(object):
     def format(self, long):
         """Formats the review thread with comments."""
         lines = []
-        # TODO: Add flag to fetch/print diffHunk for more context.
         lines.append(
             "line %d; %s"
             % (self.line, ("resolved" if self.is_resolved else "unresolved"),)
         )
-        # TODO: Try to link to the review thread with an appropriate diff.
-        # Ideally comment-to-present, worst case original-to-comment (to see
-        # comment). Possibly both.
-        lines.append("    %s" % self.url)
+        if self.diff_url:
+            lines.append("    COMMENT: %s" % self.comment_url)
+            lines.append("    CHANGES: %s" % self.diff_url)
+        else:
+            lines.append("    %s" % self.conversation_url)
         for comment in self.comments:
             lines.append(comment.format(long, 2))
         return "\n".join(lines)
@@ -277,7 +311,7 @@ def _accumulate_comments(parsed_args, comments, raw_comments):
 def _accumulate_threads(parsed_args, threads_by_path, raw_threads):
     """Adds threads to threads_by_path for later sorting."""
     for raw_thread in raw_threads:
-        thread = _Thread(raw_thread)
+        thread = _Thread(parsed_args, raw_thread)
 
         # Optionally skip resolved threads.
         if not parsed_args.include_resolved and thread.is_resolved:

src/scripts/pr_comments_test.py

@@ -90,7 +90,11 @@ class TestPRComments(unittest.TestCase):
 
     @staticmethod
     def fake_thread(**kwargs):
-        return pr_comments._Thread(TestPRComments.fake_thread_dict(**kwargs))
+        with mock.patch.dict(os.environ, {}):
+            parsed_args = pr_comments._parse_args(["83"])
+        return pr_comments._Thread(
+            parsed_args, TestPRComments.fake_thread_dict(**kwargs)
+        )
 
     @staticmethod
     def fake_thread_dict(
@@ -98,8 +102,9 @@ class TestPRComments(unittest.TestCase):
         path="foo.md",
         line=3,
         created_at="2001-02-03T04:05:06Z",
+        original_commit=None,
     ):
-        return {
+        thread_dict = {
             "isResolved": is_resolved,
             "comments": {
                 "nodes": [
@@ -118,11 +123,17 @@ class TestPRComments(unittest.TestCase):
                     },
                 ],
             },
-            "resolvedBy": {
+        }
+        if is_resolved:
+            thread_dict["resolvedBy"] = {
                 "login": "resolver",
                 "createdAt": "2001-02-03T04:25:26Z",
-            },
-        }
+            }
+        if original_commit:
+            thread_dict["comments"]["nodes"][0]["originalCommit"] = {
+                "abbreviatedOid": original_commit,
+            }
+        return thread_dict
 
     def test_thread_format(self):
         self.assertEqual(
@@ -161,6 +172,14 @@ class TestPRComments(unittest.TestCase):
             "  resolver at 2001-02-03 04:25:\n"
             "    <resolved>",
         )
+        self.assertEqual(
+            self.fake_thread(original_commit="abcdef").format(False),
+            "line 3; unresolved\n"
+            "    COMMENT: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef#diff-d8ca3b3d314d8209367af0eea2373b6fR3\n"
+            "    CHANGES: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef..HEAD#diff-d8ca3b3d314d8209367af0eea2373b6fL3\n"
+            "  author: comment\n"
+            "  other: reply",
+        )
 
     def test_thread_lt(self):
         thread1 = self.fake_thread(line=2)