Handle PR reviews. (#134)

The main comment on a review wasn't being handled, and this change interleaves it with the PR comments. GitHub treats these as different entities.

I'm hiding reviews with no top-level comment; these didn't seem worth including empty comments.

Links are being added to PR comments to make them easier to find. This means indenting the content, so now all comments (PR or review thread) are consistently indented, removing a parameter. Also, I'm adjusting the formatting of review threads to match.

snippets for comparison:
```
https://github.com/carbon-language/carbon-lang/pull/83#issuecomment-645573273
  googlebot: All (the pull request submitter and all commit authors) CLAs are...

https://github.com/carbon-language/carbon-lang/pull/83#pullrequestreview-434377138
  chandlerc: Overall, I think this is a pretty significant improved direction...
```

and from a review thread:
```
https://github.com/carbon-language/carbon-lang/pull/83/files/9863da8#diff-8f28081abb21e65fba5a0b0b29404c78R16
  - line 16; unresolved
  - diff: https://github.com/carbon-language/carbon-lang/pull/83/files/9863da8..HEAD#diff-8f28081abb21e65fba5a0b0b29404c78L16
  josh11b: ```suggestion¶ - Block comments begin with a line starting with `/...
  jonmeow: "begin with a line starting with" phrasing is a little confusing t...
  josh11b: As I understand @zygoloid 's lexing proposal, there may be additio...
  jonmeow: Rewritten with example
```

src/scripts/pr_comments.py

@@ -21,9 +21,11 @@ _QUERY = """
       author {
         login
       }
+      createdAt
       title
 
       %(comments)s
+      %(reviews)s
       %(review_threads)s
     }
   }
@@ -47,6 +49,23 @@ _QUERY_COMMENTS = """
       }
 """
 
+_QUERY_REVIEWS = """
+      reviews(first: 100%s) {
+        nodes {
+          author {
+            login
+          }
+          body
+          createdAt
+          url
+        }
+        pageInfo {
+          endCursor
+          hasNextPage
+        }
+      }
+"""
+
 _QUERY_REVIEW_THREADS = """
       reviewThreads(first: 100%s) {
         nodes {
@@ -98,8 +117,8 @@ class _Comment(object):
         )
 
     @staticmethod
-    def _rewrap(content, indent):
-        """Rewraps a comment to fit in 80 columns with an optional indent."""
+    def _rewrap(content):
+        """Rewraps a comment to fit in 80 columns with an indent."""
         lines = []
         for line in content.split("\n"):
             lines.extend(
@@ -108,31 +127,49 @@ class _Comment(object):
                     for x in textwrap.wrap(
                         line,
                         width=80,
-                        initial_indent=" " * indent,
-                        subsequent_indent=" " * indent,
+                        initial_indent=" " * 4,
+                        subsequent_indent=" " * 4,
                     )
                 ]
             )
         return "\n".join(lines)
 
-    def format(self, long, indent):
+    def format(self, long):
         """Formats the comment."""
         if long:
             return "%s%s at %s:\n%s" % (
-                " " * indent,
+                " " * 2,
                 self.author,
                 self.timestamp.strftime("%Y-%m-%d %H:%M"),
-                self._rewrap(self.body, indent + 2),
+                self._rewrap(self.body),
             )
         else:
             # Compact newlines down into pilcrows, leaving a space after.
             body = self.body.replace("\r", "").replace("\n", "¶ ")
             while "¶ ¶" in body:
                 body = body.replace("¶ ¶", "¶¶")
-            line = "%s%s: %s" % (" " * indent, self.author, body)
+            line = "%s%s: %s" % (" " * 2, self.author, body)
             return line if len(line) <= 80 else line[:77] + "..."
 
 
+class _PRComment(_Comment):
+    """A comment on the top-level PR."""
+
+    def __init__(self, raw_comment):
+        super().__init__(
+            raw_comment["author"]["login"],
+            raw_comment["createdAt"],
+            raw_comment["body"],
+        )
+        self.url = raw_comment["url"]
+
+    def __lt__(self, other):
+        return self.timestamp < other.timestamp
+
+    def format(self, long):
+        return "%s\n%s" % (self.url, super().format(long))
+
+
 class _Thread(object):
     """A review thread on a line of code."""
 
@@ -144,36 +181,29 @@ class _Thread(object):
         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
+        # Link to the comment in the commit; 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.
+        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.url = template % format_dict
+        format_dict["head"] = "..HEAD"
+        format_dict["line_side"] = "L"
+        self.diff_url = template % format_dict
 
         self.comments = [
             _Comment.from_raw_comment(comment)
@@ -198,16 +228,17 @@ class _Thread(object):
         """Formats the review thread with comments."""
         lines = []
         lines.append(
-            "line %d; %s"
-            % (self.line, ("resolved" if self.is_resolved else "unresolved"),)
+            "%s\n  - line %d; %s"
+            % (
+                self.url,
+                self.line,
+                ("resolved" if self.is_resolved else "unresolved"),
+            )
         )
         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)
+            lines.append("  - diff: %s" % self.diff_url)
         for comment in self.comments:
-            lines.append(comment.format(long, 2))
+            lines.append(comment.format(long))
         return "\n".join(lines)
 
     def has_comment_from(self, comments_from):
@@ -285,6 +316,7 @@ def _query(parsed_args, client, field_name=None, field=None):
         "repo": parsed_args.repo,
         "comments": "",
         "review_threads": "",
+        "reviews": "",
     }
     if field:
         # Use a cursor for pagination of the field.
@@ -293,19 +325,25 @@ def _query(parsed_args, client, field_name=None, field=None):
             format_inputs["comments"] = _QUERY_COMMENTS % cursor
         elif field_name == "reviewThreads":
             format_inputs["review_threads"] = _QUERY_REVIEW_THREADS % cursor
+        elif field_name == "reviews":
+            format_inputs["reviews"] = _QUERY_REVIEWS % cursor
         else:
             raise ValueError("Unexpected field_name: %s" % field_name)
     else:
-        # Fetch the first page of both fields.
+        # Fetch the first page of all fields.
         format_inputs["comments"] = _QUERY_COMMENTS % ""
         format_inputs["review_threads"] = _QUERY_REVIEW_THREADS % ""
+        format_inputs["reviews"] = _QUERY_REVIEWS % ""
     return client.execute(gql.gql(_QUERY % format_inputs))
 
 
-def _accumulate_comments(parsed_args, comments, raw_comments):
-    """Collects top-level comments."""
+def _accumulate_pr_comments(parsed_args, comments, raw_comments):
+    """Collects top-level comments and reviews."""
     for raw_comment in raw_comments:
-        comments.append(_Comment.from_raw_comment(raw_comment))
+        # Elide reviews that have no top-level comment body.
+        if not raw_comment["body"]:
+            continue
+        comments.append(_PRComment(raw_comment))
 
 
 def _accumulate_threads(parsed_args, threads_by_path, raw_threads):
@@ -373,11 +411,20 @@ def _fetch_comments(parsed_args):
     threads_result = _query(parsed_args, client)
     pull_request = threads_result["repository"]["pullRequest"]
 
-    # Paginate comments and review threads.
+    # Paginate comments, reviews, and review threads.
     comments = []
     _paginate(
         "comments",
-        _accumulate_comments,
+        _accumulate_pr_comments,
+        parsed_args,
+        client,
+        pull_request,
+        comments,
+    )
+    # Combine reviews into comments for interleaving.
+    _paginate(
+        "reviews",
+        _accumulate_pr_comments,
         parsed_args,
         client,
         pull_request,
@@ -394,10 +441,13 @@ def _fetch_comments(parsed_args):
     )
 
     # Now that loading is done (no more progress indicators), print the header.
-    print(
-        "\n  '%s' by %s"
-        % (pull_request["title"], pull_request["author"]["login"])
+    print()
+    pr_desc = _Comment(
+        pull_request["author"]["login"],
+        pull_request["createdAt"],
+        pull_request["title"],
     )
+    print(pr_desc.format(parsed_args.long))
     return comments, threads_by_path
 
 
@@ -405,9 +455,9 @@ def main():
     parsed_args = _parse_args()
     comments, threads_by_path = _fetch_comments(parsed_args)
 
-    print()
-    for comment in comments:
-        print(comment.format(parsed_args.long, 0))
+    for comment in sorted(comments):
+        print()
+        print(comment.format(parsed_args.long))
 
     for path, threads in sorted(threads_by_path.items()):
         # Print a header for each path.

src/scripts/pr_comments_test.py

@@ -12,27 +12,19 @@ class TestPRComments(unittest.TestCase):
     def test_format_comment_short(self):
         created_at = "2001-02-03T04:05:06Z"
         self.assertEqual(
-            pr_comments._Comment("author", created_at, "brief").format(
-                False, 0
-            ),
-            "author: brief",
-        )
-        self.assertEqual(
-            pr_comments._Comment("author", created_at, "brief").format(
-                False, 2
-            ),
+            pr_comments._Comment("author", created_at, "brief").format(False),
             "  author: brief",
         )
         self.assertEqual(
             pr_comments._Comment("author", created_at, "brief\nwrap").format(
-                False, 2
+                False
             ),
             "  author: brief¶ wrap",
         )
         self.assertEqual(
             pr_comments._Comment(
                 "author", created_at, "brief\n\n\nwrap"
-            ).format(False, 2),
+            ).format(False),
             "  author: brief¶¶¶ wrap",
         )
         self.assertEqual(
@@ -41,7 +33,7 @@ class TestPRComments(unittest.TestCase):
                 created_at,
                 "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed "
                 "do eiusmo",
-            ).format(False, 2),
+            ).format(False),
             "  author: Lorem ipsum dolor sit amet, consectetur adipiscing "
             "elit, sed do eiusmo",
         )
@@ -51,7 +43,7 @@ class TestPRComments(unittest.TestCase):
                 created_at,
                 "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed "
                 "do eiusmod",
-            ).format(False, 2),
+            ).format(False),
             "  author: Lorem ipsum dolor sit amet, consectetur adipiscing "
             "elit, sed do eiu...",
         )
@@ -59,16 +51,12 @@ class TestPRComments(unittest.TestCase):
     def test_format_comment_long(self):
         created_at = "2001-02-03T04:05:06Z"
         self.assertEqual(
-            pr_comments._Comment("author", created_at, "brief").format(True, 0),
-            "author at 2001-02-03 04:05:\n  brief",
-        )
-        self.assertEqual(
-            pr_comments._Comment("author", created_at, "brief").format(True, 2),
+            pr_comments._Comment("author", created_at, "brief").format(True),
             "  author at 2001-02-03 04:05:\n    brief",
         )
         self.assertEqual(
             pr_comments._Comment("author", created_at, "brief\nwrap").format(
-                True, 2
+                True
             ),
             "  author at 2001-02-03 04:05:\n    brief\n    wrap",
         )
@@ -80,7 +68,7 @@ class TestPRComments(unittest.TestCase):
             "Ut enim ad minim veniam,"
         )
         self.assertEqual(
-            pr_comments._Comment("author", created_at, body).format(True, 2),
+            pr_comments._Comment("author", created_at, body).format(True),
             "  author at 2001-02-03 04:05:\n"
             "    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed "
             "do eiusmod\n"
@@ -102,7 +90,6 @@ class TestPRComments(unittest.TestCase):
         path="foo.md",
         line=3,
         created_at="2001-02-03T04:05:06Z",
-        original_commit=None,
     ):
         thread_dict = {
             "isResolved": is_resolved,
@@ -112,6 +99,7 @@ class TestPRComments(unittest.TestCase):
                         "author": {"login": "author"},
                         "body": "comment",
                         "createdAt": created_at,
+                        "originalCommit": {"abbreviatedOid": "abcdef"},
                         "originalPosition": line,
                         "path": path,
                         "url": "http://xyz",
@@ -129,24 +117,22 @@ class TestPRComments(unittest.TestCase):
                 "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(
             self.fake_thread().format(False),
-            "line 3; unresolved\n"
-            "    http://xyz\n"
+            "https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef#diff-d8ca3b3d314d8209367af0eea2373b6fR3\n"
+            "  - line 3; unresolved\n"
+            "  - diff: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef..HEAD#diff-d8ca3b3d314d8209367af0eea2373b6fL3\n"
             "  author: comment\n"
             "  other: reply",
         )
         self.assertEqual(
             self.fake_thread().format(True),
-            "line 3; unresolved\n"
-            "    http://xyz\n"
+            "https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef#diff-d8ca3b3d314d8209367af0eea2373b6fR3\n"
+            "  - line 3; unresolved\n"
+            "  - diff: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef..HEAD#diff-d8ca3b3d314d8209367af0eea2373b6fL3\n"
             "  author at 2001-02-03 04:05:\n"
             "    comment\n"
             "  other at 2001-02-03 04:15:\n"
@@ -155,16 +141,18 @@ class TestPRComments(unittest.TestCase):
 
         self.assertEqual(
             self.fake_thread(is_resolved=True).format(False),
-            "line 3; resolved\n"
-            "    http://xyz\n"
+            "https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef#diff-d8ca3b3d314d8209367af0eea2373b6fR3\n"
+            "  - line 3; resolved\n"
+            "  - diff: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef..HEAD#diff-d8ca3b3d314d8209367af0eea2373b6fL3\n"
             "  author: comment\n"
             "  other: reply\n"
             "  resolver: <resolved>",
         )
         self.assertEqual(
             self.fake_thread(is_resolved=True).format(True),
-            "line 3; resolved\n"
-            "    http://xyz\n"
+            "https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef#diff-d8ca3b3d314d8209367af0eea2373b6fR3\n"
+            "  - line 3; resolved\n"
+            "  - diff: https://github.com/carbon-language/carbon-lang/pull/83/files/abcdef..HEAD#diff-d8ca3b3d314d8209367af0eea2373b6fL3\n"
             "  author at 2001-02-03 04:05:\n"
             "    comment\n"
             "  other at 2001-02-03 04:15:\n"
@@ -172,14 +160,6 @@ 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)
@@ -215,6 +195,66 @@ class TestPRComments(unittest.TestCase):
         self.assertEqual(threads[2].line, 4)
         self.assertEqual(len(threads_by_path["other.md"]), 1)
 
+    @staticmethod
+    def fake_pr_comment(**kwargs):
+        with mock.patch.dict(os.environ, {}):
+            parsed_args = pr_comments._parse_args(["83"])
+        return pr_comments._PRComment(
+            TestPRComments.fake_pr_comment_dict(**kwargs)
+        )
+
+    @staticmethod
+    def fake_pr_comment_dict(
+        body="comment", created_at="2001-02-03T04:05:06Z",
+    ):
+        pr_comment_dict = {
+            "author": {"login": "author"},
+            "body": body,
+            "createdAt": created_at,
+            "url": "http://xyz",
+        }
+        return pr_comment_dict
+
+    def test_pr_comment_format(self):
+        self.assertEqual(
+            self.fake_pr_comment().format(False),
+            "http://xyz\n  author: comment",
+        )
+        self.assertEqual(
+            self.fake_pr_comment().format(True),
+            "http://xyz\n  author at 2001-02-03 04:05:\n    comment",
+        )
+
+    def test_pr_comment_lt(self):
+        pr_comment1 = self.fake_pr_comment()
+        pr_comment2 = self.fake_pr_comment(created_at="2002-02-03T04:05:06Z")
+
+        self.assertTrue(pr_comment1 < pr_comment2)
+        self.assertFalse(pr_comment2 < pr_comment1)
+
+        self.assertFalse(pr_comment2 < pr_comment2)
+
+    def test_accumulate_pr_comments(self):
+        with mock.patch.dict(os.environ, {}):
+            parsed_args = pr_comments._parse_args(["83"])
+        raw_comments = [
+            self.fake_pr_comment_dict(body="x"),
+            self.fake_pr_comment_dict(body=""),
+            self.fake_pr_comment_dict(
+                body="y", created_at="2000-02-03T04:05:06Z"
+            ),
+            self.fake_pr_comment_dict(
+                body="z", created_at="2002-02-03T04:05:06Z"
+            ),
+        ]
+        comments = []
+        pr_comments._accumulate_pr_comments(parsed_args, comments, raw_comments)
+        comments.sort()
+        self.assertEqual(len(comments), 3)
+        self.assertEqual(comments[0].body, "y")
+        self.assertEqual(comments[1].body, "x")
+        self.assertEqual(comments[2].body, "z")
+
 
 if __name__ == "__main__":
     unittest.main()