feat: capture and output clang version used in feedback (#124)

also fix some errors:
1. about filter files per clang tool
2. merged suggestions' file name must match exactly
3. tool summary exclusion from PR review summary
   when tool was not used at all OR review was not requested from a particular tool

Author: 2bndy5

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()
 
 

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

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)

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

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")
 

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:

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