Some refactoring; Improve review comment content

cpp_linter/clang_tools/clang_tidy.py

@@ -94,6 +94,15 @@ def __init__(self, notes: List[TidyNotification]) -> None:
         self.patched: Optional[bytes] = None
         self.notes = notes
 
+    def diagnostics_in_range(self, start: int, end: int) -> str:
+        """Get a markdown formatted list of diagnostics found between a ``start``
+        and ``end`` range of lines."""
+        diagnostics = ""
+        for note in self.notes:
+            if note.line in range(start, end + 1):  # range is inclusive
+                diagnostics += f"- {note.rationale} [{note.diagnostic_link}]\n"
+        return diagnostics
+
 
 def run_clang_tidy(
     command: str,

cpp_linter/common_fs.py

@@ -100,7 +100,17 @@ def serialize(self) -> Dict[str, Any]:
         }
 
     def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]:
-        """Is a given ``start`` and ``end`` line numbers within a single diff hunk?"""
+        """Does a given ``hunk`` start and end within a single diff hunk?
+
+        This also includes some compensations for hunk headers that are oddly formed.
+
+        .. tip:: This is mostly useful to create comments that can be posted within a
+            git changes' diff. Ideally, designed for PR reviews based on patches
+            generated by clang tools' output.
+
+        :returns: The appropriate starting and ending line numbers of the given hunk.
+            If hunk cannot fit in a single hunk, this returns `None`.
+        """
         if hunk.old_lines > 0:
             start = hunk.old_start
             # span of old_lines is an inclusive range

cpp_linter/rest_api/__init__.py

@@ -8,6 +8,13 @@
 from ..loggers import logger
 
 
+USER_OUTREACH = (
+    "\n\nHave any feedback or feature suggestions? [Share it here.]"
+    + "(https://github.com/cpp-linter/cpp-linter-action/issues)"
+)
+COMMENT_MARKER = "<!-- cpp linter action -->\n"
+
+
 class RestApiClient(ABC):
     def __init__(self) -> None:
         self.session = requests.Session()
@@ -114,7 +121,7 @@ def make_comment(
                 else:
                     logger.debug("%s != %s", file_obj.name, note.filename)
 
-        comment = "<!-- cpp linter action -->\n# Cpp-Linter Report "
+        comment = f"{COMMENT_MARKER}# Cpp-Linter Report "
         if format_comment or tidy_comment:
             comment += ":warning:\nSome files did not pass the configured checks!\n"
             if format_comment:
@@ -127,10 +134,7 @@ def make_comment(
                 comment += f"{tidy_comment}\n</details>"
         else:
             comment += ":heavy_check_mark:\nNo problems need attention."
-        comment += (
-            "\n\nHave any feedback or feature suggestions? [Share it here.]"
-            + "(https://github.com/cpp-linter/cpp-linter-action/issues)"
-        )
+        comment += USER_OUTREACH
         return (comment, format_checks_failed, tidy_checks_failed)
 
     def post_feedback(

cpp_linter/rest_api/github_api.py

@@ -21,7 +21,7 @@
 from ..clang_tools.clang_tidy import TidyAdvice
 from ..loggers import start_log_group, logger, log_response_msg, log_commander
 from ..git import parse_diff, get_diff
-from . import RestApiClient
+from . import RestApiClient, USER_OUTREACH, COMMENT_MARKER
 
 
 class GithubApiClient(RestApiClient):
@@ -323,7 +323,7 @@ def remove_bot_comments(
             for comment in comments:
                 # only search for comments that begin with a specific html comment.
                 # the specific html comment is our action's name
-                if cast(str, comment["body"]).startswith("<!-- cpp linter action -->"):
+                if cast(str, comment["body"]).startswith(COMMENT_MARKER):
                     logger.debug(
                         "comment id %d from user %s (%d)",
                         comment["id"],
@@ -367,7 +367,7 @@ def post_review(
         self._dismiss_stale_reviews(url)
         if is_draft:
             return  # don't post reviews for PRs marked as "draft"
-        body = "<!-- cpp-linter-action -->\n## Cpp-linter Review\n"
+        body = f"{COMMENT_MARKER}## Cpp-linter Review\n"
         payload_comments = []
         total_changes = 0
         for index, tool_advice in enumerate([format_advice, tidy_advice]):
@@ -384,10 +384,14 @@ def post_review(
             if patch:
                 body += f"\n<details><summary>Click here for the full {tool} patch"
                 body += f"</summary>\n\n\n```diff\n{patch}\n```\n\n\n</details>\n\n"
+            else:
+                body += f"No objections from {tool}."
         if total_changes:
             event = "REQUEST_CHANGES"
         else:
+            body += "\nGreat job!"
             event = "APPROVE"
+        body += USER_OUTREACH
         payload = {
             "body": body,
             "event": event,
@@ -428,12 +432,7 @@ def create_review_comments(
                 body = ""
                 if isinstance(advice, TidyAdvice):
                     body += "### clang-tidy "
-                    diagnostics = ""
-                    for note in advice.notes:
-                        if note.line in range(start_lines, end_lines + 1):
-                            diagnostics += (
-                                f"- {note.rationale} [{note.diagnostic_link}]\n"
-                            )
+                    diagnostics = advice.diagnostics_in_range(start_lines, end_lines)
                     if diagnostics:
                         body += "diagnostics\n" + diagnostics
                     else:
@@ -470,9 +469,7 @@ def _dismiss_stale_reviews(self, url: str):
             for review in reviews:
                 if (
                     "body" in review
-                    and cast(str, review["body"]).startswith(
-                        "<!-- cpp-linter-action -->\n"
-                    )
+                    and cast(str, review["body"]).startswith(COMMENT_MARKER)
                     and "state" in review
                     and review["state"] not in ["PENDING", "DISMISSED"]
                 ):

tests/reviews/pr_reviews.json

@@ -22,7 +22,7 @@
       "type": "Bot",
       "site_admin": false
     },
-    "body": "<!-- cpp-linter-action -->\n## Cpp-linter Review\nOnly 1 out of 4 clang-format suggestions fit within this pull request's diff.\n\n<details><summary>Click here for the full clang-format patch</summary>\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..c522998 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -4,9 +4,7 @@\n \r\n+int main()\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n-    for (;;) break;\r\n-\r\n+    for (;;)\r\n+        break;\r\n \r\n@@ -14,5 +12,3 @@ int main(){\n \r\n-\r\n-\r\n-\r\n-    return 0;}\r\n+    return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..8f92cac 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -7,25 +7,12 @@ class Dummy {\n     int numb;\r\n-    Dummy() :numb(0), useless(\"\\0\"){}\r\n+    Dummy()\r\n+        : numb(0)\r\n+        , useless(\"\\0\")\r\n+    {\r\n+    }\r\n \r\n     public:\r\n-    void *not_useful(char *str){useless = str;}\r\n+        void* not_useful(char* str) { useless = str; }\r\n };\r\n \r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n struct LongDiff\r\n@@ -33,4 +20,3 @@ struct LongDiff\n \r\n-    long diff;\r\n-\r\n+        long diff;\r\n };\r\n\n```\n\n\n</details>\n\nOnly 2 out of 3 clang-tidy suggestions fit within this pull request's diff.\n\n<details><summary>Click here for the full clang-tidy patch</summary>\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..b160609 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -2,11 +2,10 @@\n #include \"demo.hpp\"\r\n-#include <stdio.h>\r\n+#include <cstdio>\r\n \r\n+auto main() -> int\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n-    for (;;) break;\r\n-\r\n+    for (;;) {\r\n+        break;\r\n+    }\r\n \r\n@@ -17,2 +16,3 @@ int main(){\n \r\n-    return 0;}\r\n+    return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..2591c48 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -10,3 +10,3 @@ class Dummy {\n     public:\r\n-    void *not_useful(char *str){useless = str;}\r\n+        auto not_useful(char* str) -> void* { useless = str; }\r\n };\r\n\n```\n\n\n</details>\n\n",
+    "body": "<!-- cpp linter action -->\n## Cpp-linter Review\nOnly 1 out of 4 clang-format suggestions fit within this pull request's diff.\n\n<details><summary>Click here for the full clang-format patch</summary>\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..c522998 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -4,9 +4,7 @@\n \r\n+int main()\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n-    for (;;) break;\r\n-\r\n+    for (;;)\r\n+        break;\r\n \r\n@@ -14,5 +12,3 @@ int main(){\n \r\n-\r\n-\r\n-\r\n-    return 0;}\r\n+    return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..8f92cac 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -7,25 +7,12 @@ class Dummy {\n     int numb;\r\n-    Dummy() :numb(0), useless(\"\\0\"){}\r\n+    Dummy()\r\n+        : numb(0)\r\n+        , useless(\"\\0\")\r\n+    {\r\n+    }\r\n \r\n     public:\r\n-    void *not_useful(char *str){useless = str;}\r\n+        void* not_useful(char* str) { useless = str; }\r\n };\r\n \r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n-\r\n struct LongDiff\r\n@@ -33,4 +20,3 @@ struct LongDiff\n \r\n-    long diff;\r\n-\r\n+        long diff;\r\n };\r\n\n```\n\n\n</details>\n\nOnly 2 out of 3 clang-tidy suggestions fit within this pull request's diff.\n\n<details><summary>Click here for the full clang-tidy patch</summary>\n\n\n```diff\ndiff --git a/src/demo.cpp b/src/demo.cpp\nindex fc295c3..b160609 100644\n--- a/src/demo.cpp\n+++ b/src/demo.cpp\n@@ -2,11 +2,10 @@\n #include \"demo.hpp\"\r\n-#include <stdio.h>\r\n+#include <cstdio>\r\n \r\n+auto main() -> int\r\n+{\r\n \r\n-\r\n-\r\n-int main(){\r\n-\r\n-    for (;;) break;\r\n-\r\n+    for (;;) {\r\n+        break;\r\n+    }\r\n \r\n@@ -17,2 +16,3 @@ int main(){\n \r\n-    return 0;}\r\n+    return 0;\r\n+}\r\ndiff --git a/src/demo.hpp b/src/demo.hpp\nindex a429f5c..2591c48 100644\n--- a/src/demo.hpp\n+++ b/src/demo.hpp\n@@ -10,3 +10,3 @@ class Dummy {\n     public:\r\n-    void *not_useful(char *str){useless = str;}\r\n+        auto not_useful(char* str) -> void* { useless = str; }\r\n };\r\n\n```\n\n\n</details>\n\n",
     "state": "CHANGES_REQUESTED",
     "html_url": "https://github.com/cpp-linter/test-cpp-linter-action/pull/27#pullrequestreview-1807607546",
     "pull_request_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27",

tests/reviews/test_pr_review.py

@@ -18,13 +18,21 @@
 @pytest.mark.parametrize(
     "format_review", [True, False], ids=["yes_format", "no_format"]
 )
+@pytest.mark.parametrize(
+    "force_approved", [True, False], ids=["approved", "request_changes"]
+)
+@pytest.mark.parametrize(
+    "lines_changed_only", [0, 1, 2], ids=["all_lines", "lines_added", "diff_lines"]
+)
 def test_post_review(
     monkeypatch: pytest.MonkeyPatch,
     tmp_path: Path,
     is_draft: bool,
     with_token: bool,
     tidy_review: bool,
     format_review: bool,
+    force_approved: bool,
+    lines_changed_only: int,
 ):
     """A mock test of posting PR reviews"""
     # patch env vars
@@ -82,24 +90,28 @@ def test_post_review(
             extensions=["cpp", "hpp"],
             ignored=[],
             not_ignored=[],
-            lines_changed_only=1,
+            lines_changed_only=lines_changed_only,
         )
         assert files
         for file_obj in files:
-            assert file_obj.additions
+            assert file_obj.diff_chunks
+        if force_approved:
+            files.clear()
+
         format_advice, tidy_advice = capture_clang_tools_output(
             files,
             version=environ.get("CLANG_VERSION", "16"),
             checks="",
             style="file",
-            lines_changed_only=1,
+            lines_changed_only=lines_changed_only,
             database="",
             extra_args=[],
             tidy_review=tidy_review,
             format_review=format_review,
         )
-        assert [note for concern in tidy_advice for note in concern.notes]
-        assert [note for note in format_advice]
+        if not force_approved:
+            assert [note for concern in tidy_advice for note in concern.notes]
+            assert [note for note in format_advice]
 
         # simulate draft PR by changing the request response
         cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text(
@@ -125,18 +137,26 @@ def test_post_review(
             tidy_review=tidy_review,
             format_review=format_review,
         )
-        # save the body of the review json for manual inspection
+
+        # inspect the review payload for correctness
         last_request = mock.last_request
         if (tidy_review or format_review) and not is_draft and with_token:
             assert hasattr(last_request, "json")
             json_payload = last_request.json()
             assert "body" in json_payload
+            assert "event" in json_payload
             if tidy_review:
                 assert "clang-tidy" in json_payload["body"]
             elif format_review:
                 assert "clang-format" in json_payload["body"]
             else:  # pragma: no cover
-                raise RuntimeError("no review payload sent!")
+                raise RuntimeError("review payload is incorrect")
+            if force_approved:
+                assert json_payload["event"] == "APPROVE"
+            else:
+                assert json_payload["event"] == "REQUEST_CHANGES"
+
+            # save the body of the review json for manual inspection
             assert hasattr(last_request, "text")
             (tmp_path / "review.json").write_text(
                 json.dumps(json_payload, indent=2), encoding="utf-8"