cpp-linter · 2bndy5 · Oct 5, 2024 · Oct 4, 2024 · Oct 5, 2024 · Oct 5, 2024
diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py
@@ -75,10 +75,10 @@ def main():
         )
     end_log_group()
 
-    capture_clang_tools_output(files=files, args=args)
+    clang_versions = capture_clang_tools_output(files=files, args=args)
 
     start_log_group("Posting comment(s)")
-    rest_api_client.post_feedback(files=files, args=args)
+    rest_api_client.post_feedback(files=files, args=args, clang_versions=clang_versions)
     end_log_group()
 
 

diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py
@@ -1,9 +1,9 @@
 from concurrent.futures import ProcessPoolExecutor, as_completed
 import json
 from pathlib import Path
+import re
 import subprocess
-from textwrap import indent
-from typing import Optional, List, Dict, Tuple
+from typing import Optional, List, Dict, Tuple, cast
 import shutil
 
 from ..common_fs import FileObj
@@ -77,38 +77,59 @@ def _run_on_single_file(
     return file.name, log_stream.getvalue(), tidy_note, format_advice
 
 
-def capture_clang_tools_output(files: List[FileObj], args: Args):
+VERSION_PATTERN = re.compile(r"version\s(\d+\.\d+\.\d+)")
+
+
+def _capture_tool_version(cmd: str) -> str:
+    """Get version number from output for executable used."""
+    version_out = subprocess.run(
+        [cmd, "--version"], capture_output=True, check=True, text=True
+    )
+    matched = VERSION_PATTERN.search(version_out.stdout)
+    if matched is None:  # pragma: no cover
+        raise RuntimeError(
+            f"Failed to get version numbers from `{cmd} --version` output"
+        )
+    ver = cast(str, matched.group(1))
+    logger.info("`%s --version`: %s", cmd, ver)
+    return ver
+
+
+class ClangVersions:
+    def __init__(self) -> None:
+        self.tidy: Optional[str] = None
+        self.format: Optional[str] = None
+
+
+def capture_clang_tools_output(files: List[FileObj], args: Args) -> ClangVersions:
     """Execute and capture all output from clang-tidy and clang-format. This aggregates
     results in the :attr:`~cpp_linter.Globals.OUTPUT`.
 
     :param files: A list of files to analyze.
     :param args: A namespace of parsed args from the :doc:`CLI <../cli_args>`.
     """
 
-    def show_tool_version_output(cmd: str):  # show version output for executable used
-        version_out = subprocess.run(
-            [cmd, "--version"], capture_output=True, check=True
-        )
-        logger.info("%s --version\n%s", cmd, indent(version_out.stdout.decode(), "\t"))
-
     tidy_cmd, format_cmd = (None, None)
     tidy_filter, format_filter = (None, None)
+    clang_versions = ClangVersions()
     if args.style:  # if style is an empty value, then clang-format is skipped
         format_cmd = assemble_version_exec("clang-format", args.version)
-        assert format_cmd is not None, "clang-format executable was not found"
-        show_tool_version_output(format_cmd)
-        tidy_filter = TidyFileFilter(
+        if format_cmd is None:  # pragma: no cover
+            raise FileNotFoundError("clang-format executable was not found")
+        clang_versions.format = _capture_tool_version(format_cmd)
+        format_filter = FormatFileFilter(
             extensions=args.extensions,
-            ignore_value=args.ignore_tidy,
+            ignore_value=args.ignore_format,
         )
     if args.tidy_checks != "-*":
         # if all checks are disabled, then clang-tidy is skipped
         tidy_cmd = assemble_version_exec("clang-tidy", args.version)
-        assert tidy_cmd is not None, "clang-tidy executable was not found"
-        show_tool_version_output(tidy_cmd)
-        format_filter = FormatFileFilter(
+        if tidy_cmd is None:  # pragma: no cover
+            raise FileNotFoundError("clang-tidy executable was not found")
+        clang_versions.tidy = _capture_tool_version(tidy_cmd)
+        tidy_filter = TidyFileFilter(
             extensions=args.extensions,
-            ignore_value=args.ignore_format,
+            ignore_value=args.ignore_tidy,
         )
 
     db_json: Optional[List[Dict[str, str]]] = None
@@ -155,3 +176,4 @@ def show_tool_version_output(cmd: str):  # show version output for executable us
                         break
                 else:  # pragma: no cover
                     raise ValueError(f"Failed to find {file_name} in list of files.")
+    return clang_versions
diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py
@@ -133,12 +133,16 @@ def get_suggestions_from_patch(
 
         def _has_related_suggestion(suggestion: Suggestion) -> bool:
             for known in review_comments.suggestions:
-                if known.line_end >= suggestion.line_end >= known.line_start:
+                if (
+                    known.file_name == suggestion.file_name
+                    and known.line_end >= suggestion.line_end >= known.line_start
+                ):
                     known.comment += f"\n{suggestion.comment}"
                     return True
             return False
 
         # now check for clang-tidy warnings with no fixes applied
+        assert isinstance(review_comments.tool_total["clang-tidy"], int)
         for note in self.notes:
             if not note.applied_fixes:  # if no fix was applied
                 line_numb = int(note.line)

diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py
@@ -50,12 +50,17 @@ def __init__(self) -> None:
         #: The list of actual comments
         self.suggestions: List[Suggestion] = []
 
-        self.tool_total: Dict[str, int] = {"clang-tidy": 0, "clang-format": 0}
+        self.tool_total: Dict[str, Optional[int]] = {
+            "clang-tidy": None,
+            "clang-format": None,
+        }
         """The total number of concerns about a specific clang tool.
 
         This may not equate to the length of `suggestions` because
         1. There is no guarantee that all suggestions will fit within the PR's diff.
         2. Suggestions are a combined result of advice from both tools.
+
+        A `None` value means a review was not requested from the corresponding tool.
         """
 
         self.full_patch: Dict[str, str] = {"clang-tidy": "", "clang-format": ""}
@@ -69,17 +74,26 @@ def merge_similar_suggestion(self, suggestion: Suggestion) -> bool:
         """
         for known in self.suggestions:
             if (
-                known.line_end == suggestion.line_end
+                known.file_name == suggestion.file_name
+                and known.line_end == suggestion.line_end
                 and known.line_start == suggestion.line_start
             ):
                 known.comment += f"\n{suggestion.comment}"
                 return True
         return False
 
-    def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]:
+    def serialize_to_github_payload(
+        # avoid circular imports by accepting primitive types (instead of ClangVersions)
+        self,
+        tidy_version: Optional[str],
+        format_version: Optional[str],
+    ) -> Tuple[str, List[Dict[str, Any]]]:
         """Serialize this object into a summary and list of comments compatible
         with Github's REST API.
 
+        :param tidy_version: The version numbers of the clang-tidy used.
+        :param format_version: The version numbers of the clang-format used.
+
         :returns: The returned tuple contains a brief summary (at index ``0``)
             that contains markdown text describing the summary of the review
             comments.
@@ -98,6 +112,12 @@ def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]:
                 posted_tool_advice["clang-tidy"] += 1
 
         for tool_name in ("clang-tidy", "clang-format"):
+            tool_version = tidy_version
+            if tool_name == "clang-format":
+                tool_version = format_version
+            if tool_version is None or self.tool_total[tool_name] is None:
+                continue  # if tool wasn't used
+            summary += f"### Used {tool_name} v{tool_version}\n\n"
             if (
                 len(comments)
                 and posted_tool_advice[tool_name] != self.tool_total[tool_name]
@@ -115,8 +135,7 @@ def serialize_to_github_payload(self) -> Tuple[str, List[Dict[str, Any]]]:
                 )
             elif not self.tool_total[tool_name]:
                 summary += f"No concerns from {tool_name}.\n"
-        result = (summary, comments)
-        return result
+        return (summary, comments)
 
 
 class PatchMixin(ABC):
@@ -164,8 +183,9 @@ def get_suggestions_from_patch(
         assert tool_name in review_comments.full_patch
         review_comments.full_patch[tool_name] += f"{patch.text}"
         assert tool_name in review_comments.tool_total
+        tool_total = review_comments.tool_total[tool_name] or 0
         for hunk in patch.hunks:
-            review_comments.tool_total[tool_name] += 1
+            tool_total += 1
             if summary_only:
                 continue
             new_hunk_range = file_obj.is_hunk_contained(hunk)
@@ -193,3 +213,5 @@ def get_suggestions_from_patch(
             comment.comment = body
             if not review_comments.merge_similar_suggestion(comment):
                 review_comments.suggestions.append(comment)
+
+        review_comments.tool_total[tool_name] = tool_total
diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py
@@ -12,6 +12,7 @@
 from ..common_fs.file_filter import FileFilter
 from ..cli import Args
 from ..loggers import logger, log_response_msg
+from ..clang_tools import ClangVersions
 
 
 USER_OUTREACH = (
@@ -168,6 +169,7 @@ def make_comment(
         files: List[FileObj],
         format_checks_failed: int,
         tidy_checks_failed: int,
+        clang_versions: ClangVersions,
         len_limit: Optional[int] = None,
     ) -> str:
         """Make an MarkDown comment from the given advice. Also returns a count of
@@ -176,6 +178,7 @@ def make_comment(
         :param files: A list of objects, each describing a file's information.
         :param format_checks_failed: The amount of clang-format checks that have failed.
         :param tidy_checks_failed: The amount of clang-tidy checks that have failed.
+        :param clang_versions: The versions of the clang tools used.
         :param len_limit: The length limit of the comment generated.
 
         :Returns: The markdown comment as a `str`
@@ -199,12 +202,14 @@ def adjust_limit(limit: Optional[int], text: str) -> Optional[int]:
                     files=files,
                     checks_failed=format_checks_failed,
                     len_limit=len_limit,
+                    version=clang_versions.format,
                 )
             if tidy_checks_failed:
                 comment += RestApiClient._make_tidy_comment(
                     files=files,
                     checks_failed=tidy_checks_failed,
                     len_limit=adjust_limit(limit=len_limit, text=comment),
+                    version=clang_versions.tidy,
                 )
         else:
             prefix = ":heavy_check_mark:\nNo problems need attention."
@@ -215,9 +220,12 @@ def _make_format_comment(
         files: List[FileObj],
         checks_failed: int,
         len_limit: Optional[int] = None,
+        version: Optional[str] = None,
     ) -> str:
         """make a comment describing clang-format errors"""
-        comment = "\n<details><summary>clang-format reports: <strong>"
+        comment = "\n<details><summary>clang-format{} reports: <strong>".format(
+            "" if version is None else f" (v{version})"
+        )
         comment += f"{checks_failed} file(s) not formatted</strong></summary>\n\n"
         closer = "\n</details>"
         checks_failed = 0
@@ -238,9 +246,12 @@ def _make_tidy_comment(
         files: List[FileObj],
         checks_failed: int,
         len_limit: Optional[int] = None,
+        version: Optional[str] = None,
     ) -> str:
         """make a comment describing clang-tidy errors"""
-        comment = "\n<details><summary>clang-tidy reports: <strong>"
+        comment = "\n<details><summary>clang-tidy{} reports: <strong>".format(
+            "" if version is None else f" (v{version})"
+        )
         comment += f"{checks_failed} concern(s)</strong></summary>\n\n"
         closer = "\n</details>"
         for file_obj in files:
@@ -276,11 +287,13 @@ def post_feedback(
         self,
         files: List[FileObj],
         args: Args,
+        clang_versions: ClangVersions,
     ):
         """Post action's results using REST API.
 
         :param files: A list of objects, each describing a file's information.
         :param args: A namespace of arguments parsed from the :doc:`CLI <../cli_args>`.
+        :param clang_versions: The version of the clang tools used.
         """
         raise NotImplementedError("Must be defined in the derivative")
 

diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py
@@ -25,6 +25,7 @@
 )
 from ..clang_tools.clang_tidy import tally_tidy_advice
 from ..clang_tools.patcher import ReviewComments, PatchMixin
+from ..clang_tools import ClangVersions
 from ..cli import Args
 from ..loggers import logger, log_commander
 from ..git import parse_diff, get_diff
@@ -187,6 +188,7 @@ def post_feedback(
         self,
         files: List[FileObj],
         args: Args,
+        clang_versions: ClangVersions,
     ):
         format_checks_failed = tally_format_advice(files)
         tidy_checks_failed = tally_tidy_advice(files)
@@ -198,6 +200,7 @@ def post_feedback(
                 files=files,
                 format_checks_failed=format_checks_failed,
                 tidy_checks_failed=tidy_checks_failed,
+                clang_versions=clang_versions,
                 len_limit=None,
             )
             with open(environ["GITHUB_STEP_SUMMARY"], "a", encoding="utf-8") as summary:
@@ -225,6 +228,7 @@ def post_feedback(
                     files=files,
                     format_checks_failed=format_checks_failed,
                     tidy_checks_failed=tidy_checks_failed,
+                    clang_versions=clang_versions,
                     len_limit=65535,
                 )
 
@@ -253,6 +257,7 @@ def post_feedback(
                 format_review=args.format_review,
                 no_lgtm=args.no_lgtm,
                 passive_reviews=args.passive_reviews,
+                clang_versions=clang_versions,
             )
 
     def make_annotations(
@@ -388,6 +393,7 @@ def post_review(
         format_review: bool,
         no_lgtm: bool,
         passive_reviews: bool,
+        clang_versions: ClangVersions,
     ):
         url = f"{self.api_url}/repos/{self.repo}/pulls/{self.pull_request}"
         response = self.api_request(url=url)
@@ -419,11 +425,15 @@ def post_review(
                 summary_only=summary_only,
                 review_comments=review_comments,
             )
-        (summary, comments) = review_comments.serialize_to_github_payload()
+        (summary, comments) = review_comments.serialize_to_github_payload(
+            # avoid circular imports by passing primitive types
+            tidy_version=clang_versions.tidy,
+            format_version=clang_versions.format,
+        )
         if not summary_only:
             payload_comments.extend(comments)
         body += summary
-        if sum(review_comments.tool_total.values()):
+        if sum([x for x in review_comments.tool_total.values() if isinstance(x, int)]):
             event = "REQUEST_CHANGES"
         else:
             if no_lgtm:
@@ -457,6 +467,8 @@ def create_review_comments(
         :param review_comments: An object (passed by reference) that is used to store
             the results.
         """
+        tool_name = "clang-tidy" if tidy_tool else "clang-format"
+        review_comments.tool_total[tool_name] = 0
         for file_obj in files:
             tool_advice: Optional[PatchMixin]
             if tidy_tool:

diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py
@@ -11,7 +11,7 @@
 from cpp_linter.loggers import logger
 from cpp_linter.common_fs import FileObj, CACHE_PATH
 from cpp_linter.rest_api.github_api import GithubApiClient
-from cpp_linter.clang_tools import capture_clang_tools_output
+from cpp_linter.clang_tools import ClangVersions, capture_clang_tools_output
 from cpp_linter.clang_tools.clang_format import tally_format_advice
 from cpp_linter.clang_tools.clang_tidy import tally_tidy_advice
 from cpp_linter.cli import Args
@@ -96,7 +96,7 @@ def test_ninja_database(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
     args.lines_changed_only = 0  # analyze complete file
 
     # run clang-tidy and verify paths of project files were matched with database paths
-    capture_clang_tools_output(files, args=args)
+    clang_versions: ClangVersions = capture_clang_tools_output(files, args=args)
     found_project_file = False
     for concern in [a.tidy_advice for a in files if a.tidy_advice]:
         for note in concern.notes:
@@ -112,6 +112,7 @@ def test_ninja_database(monkeypatch: pytest.MonkeyPatch, tmp_path: Path):
         files=files,
         tidy_checks_failed=tidy_checks_failed,
         format_checks_failed=format_checks_failed,
+        clang_versions=clang_versions,
     )
 
     assert tidy_checks_failed