PR Review Suggestions

.pre-commit-config.yaml

@@ -3,7 +3,7 @@ repos:
     rev: v4.5.0
     hooks:
       - id: trailing-whitespace
-        exclude: tests/capture_tools_output/cpp-linter/cpp-linter/test_git_lib.patch
+        exclude: ^tests/.*\.(?:patch|diff)$
       - id: end-of-file-fixer
       - id: check-docstring-first
       - id: check-added-large-files

codecov.yml

@@ -0,0 +1,11 @@
+coverage:
+  status:
+    patch:
+      default:
+        informational: true
+    project:
+      default:
+        target: auto
+        # adjust accordingly based on how flaky your tests are
+        # this allows a 2% drop from the previous base commit coverage
+        threshold: 2%

cpp_linter/__init__.py

@@ -23,6 +23,7 @@ def main():
 
     rest_api_client = GithubApiClient()
     logger.info("processing %s event", rest_api_client.event_name)
+    is_pr_event = rest_api_client.event_name == "pull_request"
 
     # set logging verbosity
     logger.setLevel(10 if args.verbosity or rest_api_client.debug_enabled else 20)
@@ -46,10 +47,27 @@ def main():
             not_ignored,
             args.lines_changed_only,
         )
-        if files:
-            rest_api_client.verify_files_are_present(files)
+        rest_api_client.verify_files_are_present(files)
     else:
         files = list_source_files(args.extensions, ignored, not_ignored)
+        # at this point, files have no info about git changes.
+        # for PR reviews, we need this info
+        if is_pr_event and (args.tidy_review or args.format_review):
+            # get file changes from diff
+            git_changes = rest_api_client.get_list_of_changed_files(
+                args.extensions,
+                ignored,
+                not_ignored,
+                lines_changed_only=0,  # prevent filtering out unchanged files
+            )
+            # merge info from git changes into list of all files
+            for git_file in git_changes:
+                for file in files:
+                    if git_file.name == file.name:
+                        file.additions = git_file.additions
+                        file.diff_chunks = git_file.diff_chunks
+                        file.lines_added = git_file.lines_added
+                        break
     if not files:
         logger.info("No source files need checking!")
     else:
@@ -67,6 +85,8 @@ def main():
         args.lines_changed_only,
         args.database,
         args.extra_arg,
+        is_pr_event and args.tidy_review,
+        is_pr_event and args.format_review,
     )
 
     start_log_group("Posting comment(s)")
@@ -79,6 +99,8 @@ def main():
         args.step_summary,
         args.file_annotations,
         args.style,
+        args.format_review,
+        args.tidy_review,
     )
     end_log_group()
 

cpp_linter/clang_tools/__init__.py

@@ -39,6 +39,8 @@ def capture_clang_tools_output(
     lines_changed_only: int,
     database: str,
     extra_args: List[str],
+    tidy_review: bool,
+    format_review: bool,
 ) -> Tuple[List[FormatAdvice], List[TidyAdvice]]:
     """Execute and capture all output from clang-tidy and clang-format. This aggregates
     results in the :attr:`~cpp_linter.Globals.OUTPUT`.
@@ -54,6 +56,10 @@ def capture_clang_tools_output(
     :param database: The path to the compilation database.
     :param extra_args: A list of extra arguments used by clang-tidy as compiler
         arguments.
+    :param tidy_review: A flag to enable/disable creating a diff suggestion for
+        PR review comments using clang-tidy.
+    :param format_review: A flag to enable/disable creating a diff suggestion for
+        PR review comments using clang-format.
     """
 
     def show_tool_version_output(cmd: str):  # show version output for executable used
@@ -81,7 +87,7 @@ def show_tool_version_output(cmd: str):  # show version output for executable us
             db_json = json.loads(db_path.read_text(encoding="utf-8"))
 
     # temporary cache of parsed notifications for use in log commands
-    tidy_notes: List[TidyAdvice] = []
+    tidy_notes = []
     format_advice = []
     for file in files:
         start_log_group(f"Performing checkup on {file.name}")
@@ -95,11 +101,14 @@ def show_tool_version_output(cmd: str):  # show version output for executable us
                     database,
                     extra_args,
                     db_json,
+                    tidy_review,
                 )
             )
         if format_cmd is not None:
             format_advice.append(
-                run_clang_format(format_cmd, file, style, lines_changed_only)
+                run_clang_format(
+                    format_cmd, file, style, lines_changed_only, format_review
+                )
             )
         end_log_group()
     return (format_advice, tidy_notes)

cpp_linter/clang_tools/clang_format.py

@@ -1,8 +1,10 @@
 """Parse output from clang-format's XML suggestions."""
 from pathlib import PurePath
 import subprocess
-from typing import List, cast
+from typing import List, cast, Optional
+
 import xml.etree.ElementTree as ET
+
 from ..common_fs import get_line_cnt_from_cols, FileObj
 from ..loggers import logger
 
@@ -66,6 +68,9 @@ def __init__(self, filename: str):
         """A list of `FormatReplacementLine` representing replacement(s)
         on a single line."""
 
+        #: A buffer of the applied fixes from clang-format
+        self.patched: Optional[bytes] = None
+
     def __repr__(self) -> str:
         return (
             f"<XMLFixit with {len(self.replaced_lines)} lines of "
@@ -140,6 +145,7 @@ def run_clang_format(
     file_obj: FileObj,
     style: str,
     lines_changed_only: int,
+    format_review: bool,
 ) -> FormatAdvice:
     """Run clang-format on a certain file
 
@@ -149,6 +155,8 @@ def run_clang_format(
         use the relative-most .clang-format configuration file.
     :param lines_changed_only: A flag that forces focus on only changes in the event's
         diff info.
+    :param format_review: A flag to enable/disable creating a diff suggestion for
+        PR review comments.
     """
     cmds = [
         command,
@@ -168,6 +176,13 @@ def run_clang_format(
         logger.debug(
             "%s raised the following error(s):\n%s", cmds[0], results.stderr.decode()
         )
-    return parse_format_replacements_xml(
+    advice = parse_format_replacements_xml(
         results.stdout.decode(encoding="utf-8").strip(), file_obj, lines_changed_only
     )
+    if format_review:
+        del cmds[2]  # remove `--output-replacements-xml` flag
+        # get formatted file from stdout
+        formatted_output = subprocess.run(cmds, capture_output=True, check=True)
+        # store formatted_output (for comparing later)
+        advice.patched = formatted_output.stdout
+    return advice

cpp_linter/clang_tools/clang_tidy.py

@@ -5,7 +5,6 @@
 import re
 import subprocess
 from typing import Tuple, Union, List, cast, Optional, Dict
-from pygit2 import Patch  # type: ignore[import]
 from ..loggers import logger
 from ..common_fs import FileObj
 
@@ -91,10 +90,19 @@ def __repr__(self) -> str:
 
 class TidyAdvice:
     def __init__(self, notes: List[TidyNotification]) -> None:
-        #: A patch of the suggested fixes from clang-tidy
-        self.suggestion: Optional[Patch] = None
+        #: A buffer of the applied fixes from clang-tidy
+        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,
@@ -104,6 +112,7 @@ def run_clang_tidy(
     database: str,
     extra_args: List[str],
     db_json: Optional[List[Dict[str, str]]],
+    tidy_review: bool,
 ) -> TidyAdvice:
     """Run clang-tidy on a certain file.
 
@@ -134,6 +143,8 @@ def run_clang_tidy(
     :param db_json: The compilation database deserialized from JSON, only if
         ``database`` parameter points to a valid path containing a
         ``compile_commands.json file``.
+    :param tidy_review: A flag to enable/disable creating a diff suggestion for
+        PR review comments.
     """
     filename = file_obj.name.replace("/", os.sep)
     cmds = [command]
@@ -162,7 +173,21 @@ def run_clang_tidy(
         logger.debug(
             "clang-tidy made the following summary:\n%s", results.stderr.decode()
         )
-    return parse_tidy_output(results.stdout.decode(), database=db_json)
+
+    advice = parse_tidy_output(results.stdout.decode(), database=db_json)
+
+    if tidy_review:
+        # clang-tidy overwrites the file contents when applying fixes.
+        # create a cache of original contents
+        original_buf = Path(file_obj.name).read_bytes()
+        cmds.insert(1, "--fix-errors")  # include compiler-suggested fixes
+        # run clang-tidy again to apply any fixes
+        subprocess.run(cmds, check=True)
+        # store the modified output from clang-tidy
+        advice.patched = Path(file_obj.name).read_bytes()
+        # re-write original file contents (can probably skip this on CI runners)
+        Path(file_obj.name).write_bytes(original_buf)
+    return advice
 
 
 def parse_tidy_output(

cpp_linter/cli.py

@@ -281,6 +281,22 @@
 specified as positional arguments will be exempt from
 explicitly ignored domains (see :std:option:`--ignore`).""",
 )
+cli_arg_parser.add_argument(
+    "--tidy-review",
+    "-tr",
+    default="false",
+    type=lambda input: input.lower() == "true",
+    help="""Set to ``true`` to enable PR review suggestions
+from clang-tidy.""",
+)
+cli_arg_parser.add_argument(
+    "--format-review",
+    "-fr",
+    default="false",
+    type=lambda input: input.lower() == "true",
+    help="""Set to ``true`` to enable PR review suggestions
+from clang-format.""",
+)
 
 
 def parse_ignore_option(

cpp_linter/common_fs.py

@@ -1,7 +1,8 @@
 from os import environ
 from os.path import commonpath
 from pathlib import PurePath, Path
-from typing import List, Dict, Any, Union, Tuple
+from typing import List, Dict, Any, Union, Tuple, Optional
+from pygit2 import DiffHunk  # type: ignore
 from .loggers import logger, start_log_group
 
 #: A path to generated cache artifacts. (only used when verbosity is in debug mode)
@@ -19,16 +20,21 @@ class FileObj:
         for all hunks in the diff.
     """
 
-    def __init__(self, name: str, additions: List[int], diff_chunks: List[List[int]]):
+    def __init__(
+        self,
+        name: str,
+        additions: Optional[List[int]] = None,
+        diff_chunks: Optional[List[List[int]]] = None,
+    ):
         self.name: str = name  #: The file name
-        self.additions: List[int] = additions
+        self.additions: List[int] = additions or []
         """A list of line numbers that contain added changes. This will be empty if
         not focusing on lines changed only."""
-        self.diff_chunks: List[List[int]] = diff_chunks
+        self.diff_chunks: List[List[int]] = diff_chunks or []
         """A list of line numbers that define the beginning and ending of hunks in the
         diff. This will be empty if not focusing on lines changed only."""
         self.lines_added: List[List[int]] = FileObj._consolidate_list_to_ranges(
-            additions
+            additions or []
         )
         """A list of line numbers that define the beginning and ending of ranges that
         have added changes. This will be empty if not focusing on lines changed only.
@@ -93,6 +99,39 @@ def serialize(self) -> Dict[str, Any]:
             },
         }
 
+    def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]:
+        """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
+            end = hunk.old_start + hunk.old_lines - 1
+        else:  # if number of old lines is 0
+            # start hunk at new line number
+            start = hunk.new_start
+            # make it span 1 line
+            end = start
+        for hunk in self.diff_chunks:
+            chunk_range = range(hunk[0], hunk[1])
+            if start in chunk_range and end in chunk_range:
+                return (start, end)
+        logger.warning(
+            "lines %d - %d are not within a single diff hunk for file %s.",
+            start,
+            end,
+            self.name,
+        )
+        return None
+
 
 def is_file_in_list(paths: List[str], file_name: str, prompt: str) -> bool:
     """Determine if a file is specified in a list of paths and/or filenames.
@@ -195,7 +234,7 @@ def list_source_files(
                 if is_file_in_list(
                     not_ignored, file_path, "not ignored"
                 ) or not is_file_in_list(ignored, file_path, "ignored"):
-                    files.append(FileObj(file_path, [], []))
+                    files.append(FileObj(file_path))
     return files
 
 

cpp_linter/git/git_str.py

@@ -11,7 +11,7 @@
 DIFF_FILE_NAME = re.compile(r"^\+\+\+\sb?/(.*)$", re.MULTILINE)
 DIFF_RENAMED_FILE = re.compile(r"^rename to (.*)$", re.MULTILINE)
 DIFF_BINARY_FILE = re.compile(r"^Binary\sfiles\s", re.MULTILINE)
-HUNK_INFO = re.compile(r"@@\s\-\d+,\d+\s\+(\d+,\d+)\s@@", re.MULTILINE)
+HUNK_INFO = re.compile(r"^@@\s\-\d+,?\d*\s\+(\d+,?\d*)\s@@", re.MULTILINE)
 
 
 def _get_filename_from_diff(front_matter: str) -> Optional[re.Match]:
@@ -94,7 +94,11 @@ def _parse_patch(full_patch: str) -> Tuple[List[List[int]], List[int]]:
     for index, chunk in enumerate(chunks):
         if index % 2 == 1:
             # each odd element holds the starting line number and number of lines
-            start_line, hunk_length = [int(x) for x in chunk.split(",")]
+            if "," in chunk:
+                start_line, hunk_length = [int(x) for x in chunk.split(",")]
+            else:
+                start_line = int(chunk)
+                hunk_length = 1
             ranges.append([start_line, hunk_length + start_line])
             line_numb_in_diff = start_line
             continue

cpp_linter/loggers.py

@@ -51,8 +51,9 @@ def log_response_msg(response_buffer: Response) -> bool:
     """
     if response_buffer.status_code >= 400:
         logger.error(
-            "response returned %d message: %s",
+            "response returned %d from %s with message: %s",
             response_buffer.status_code,
+            response_buffer.url,
             response_buffer.text,
         )
         return False