Merge pull request #376 from Mark-Simulacrum/log-better

Improving messages sent on reply to webhooks

Author: Mark-Simulacrum

highfive/newpr.py

@@ -33,14 +33,11 @@
 review_with_reviewer = 'r? @%s\n\n(rust-highfive has picked a reviewer for you, use r? to override)'
 review_without_reviewer = '@%s: no appropriate reviewer found, use r? to override'
 
-reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)")
-reviewer_group_re = re.compile("\\b[rR]\?[:\- ]*@?(?:([a-zA-Z0-9\-]+)/)([a-zA-Z0-9\-]+)")
-submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE)
+reviewer_re = re.compile(r"\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)")
+reviewer_group_re = re.compile(r"\b[rR]\?[:\- ]*@?(?:([a-zA-Z0-9\-]+)/)([a-zA-Z0-9\-]+)")
+submodule_re = re.compile(r".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE)
 target_re = re.compile("^[+-]{3} [ab]/compiler/rustc_target/src/spec/", re.MULTILINE)
 
-rustaceans_api_url = "http://www.ncameron.org/rustaceans/user?username={username}"
-
-
 class UnsupportedRepoError(IOError):
     pass
 
@@ -68,10 +65,13 @@ def run(self, event):
             return "Ping received! The webhook is configured correctly!\n"
         elif event == "pull_request" and self.payload["action"] == "opened":
             self.new_pr()
-            return 'OK\n'
+            return 'OK, handled new PR\n'
         elif event == "issue_comment" and self.payload["action"] == "created":
-            self.new_comment()
-            return 'OK\n'
+            msg = self.new_comment()
+            if msg is None:
+                return 'OK\n'
+            else:
+                return f"OK: {msg}\n"
         else:
             return 'Unsupported webhook event.\n'
 
@@ -433,12 +433,12 @@ def new_comment(self):
         # Check the issue is a PR and is open.
         if self.payload['issue', 'state'] != 'open' \
                 or 'pull_request' not in self.payload['issue']:
-            return
+            return "skipped - closed issue"
 
         commenter = self.payload['comment', 'user', 'login']
         # Ignore our own comments.
         if commenter == self.integration_user:
-            return
+            return "skipped - our own comment"
 
         owner = self.payload['repository', 'owner', 'login']
         repo = self.payload['repository', 'name']
@@ -451,7 +451,7 @@ def new_comment(self):
         )):
             # Check if commenter is a collaborator.
             if not self.is_collaborator(commenter, owner, repo):
-                return
+                return "skipped, comment not by author, collaborator, or assignee"
 
         # Check for r? and set the assignee.
         msg = self.payload['comment', 'body']
@@ -462,3 +462,6 @@ def new_comment(self):
                 reviewer, owner, repo, issue, self.integration_user,
                 author, None
             )
+            return f"set assignee to {reviewer}"
+        else:
+            return "no reviewer found"

highfive/tests/test_newpr.py

@@ -980,7 +980,7 @@ def make_handler(
     def test_not_open(self):
         handler = self.make_handler(state='closed')
 
-        assert handler.new_comment() is None
+        assert handler.new_comment() == 'skipped - closed issue'
         self.mocks['is_collaborator'].assert_not_called()
         self.mocks['find_reviewer'].assert_not_called()
         self.mocks['set_assignee'].assert_not_called()
@@ -996,7 +996,7 @@ def test_not_pr(self):
     def test_commenter_is_integration_user(self):
         handler = self.make_handler(commenter='integrationUser')
 
-        assert handler.new_comment() is None
+        assert handler.new_comment() == 'skipped - our own comment'
         self.mocks['is_collaborator'].assert_not_called()
         self.mocks['find_reviewer'].assert_not_called()
         self.mocks['set_assignee'].assert_not_called()
@@ -1007,7 +1007,7 @@ def test_unauthorized_assigner(self):
         )
 
         self.mocks['is_collaborator'].return_value = False
-        assert handler.new_comment() is None
+        assert handler.new_comment() == 'skipped, comment not by author, collaborator, or assignee'
         self.mocks['is_collaborator'].assert_called_with(
             'userB', 'repo-owner', 'repo-name'
         )
@@ -1315,14 +1315,14 @@ def handler_mock(self, payload):
     def test_newpr(self):
         payload = Payload({'action': 'opened'})
         m = self.handler_mock(payload)
-        assert m.handler.run('pull_request') == 'OK\n'
+        assert m.handler.run('pull_request') == 'OK, handled new PR\n'
         self.mocks['new_pr'].assert_called_once_with()
         self.mocks['new_comment'].assert_not_called()
 
     def test_new_comment(self):
         payload = Payload({'action': 'created'})
         m = self.handler_mock(payload)
-        assert m.handler.run('issue_comment') == 'OK\n'
+        assert m.handler.run('issue_comment').startswith('OK')
         self.mocks['new_pr'].assert_not_called()
         self.mocks['new_comment'].assert_called_once_with()