resolves #65 (#66)

* resolves #65

This will remove the line "No objections from ..." when it is not relevant in PR review summary comments.

Also adds a 🎉 for approved PR reviews

* change logic about traversing advice for different tools

* show cmd used to get fixes in logs

* don't add quotes around extra-arg value

* correct order of arg to post_feedback()

use kwargs to avoid mis-ordering positional args

cpp_linter/__init__.py

@@ -42,10 +42,10 @@ def main():
 
     if args.files_changed_only:
         files = rest_api_client.get_list_of_changed_files(
-            args.extensions,
-            ignored,
-            not_ignored,
-            args.lines_changed_only,
+            extensions=args.extensions,
+            ignored=ignored,
+            not_ignored=not_ignored,
+            lines_changed_only=args.lines_changed_only,
         )
         rest_api_client.verify_files_are_present(files)
     else:
@@ -55,9 +55,9 @@ def main():
         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,
+                extensions=args.extensions,
+                ignored=ignored,
+                not_ignored=not_ignored,
                 lines_changed_only=0,  # prevent filtering out unchanged files
             )
             # merge info from git changes into list of all files
@@ -78,29 +78,29 @@ def main():
     end_log_group()
 
     (format_advice, tidy_advice) = capture_clang_tools_output(
-        files,
-        args.version,
-        args.tidy_checks,
-        args.style,
-        args.lines_changed_only,
-        args.database,
-        args.extra_arg,
-        is_pr_event and args.tidy_review,
-        is_pr_event and args.format_review,
+        files=files,
+        version=args.version,
+        checks=args.tidy_checks,
+        style=args.style,
+        lines_changed_only=args.lines_changed_only,
+        database=args.database,
+        extra_args=args.extra_arg,
+        tidy_review=is_pr_event and args.tidy_review,
+        format_review=is_pr_event and args.format_review,
     )
 
     start_log_group("Posting comment(s)")
     rest_api_client.post_feedback(
-        files,
-        format_advice,
-        tidy_advice,
-        args.thread_comments,
-        args.no_lgtm,
-        args.step_summary,
-        args.file_annotations,
-        args.style,
-        args.format_review,
-        args.tidy_review,
+        files=files,
+        format_advice=format_advice,
+        tidy_advice=tidy_advice,
+        thread_comments=args.thread_comments,
+        no_lgtm=args.no_lgtm,
+        step_summary=args.step_summary,
+        file_annotations=args.file_annotations,
+        style=args.style,
+        tidy_review=args.tidy_review,
+        format_review=args.format_review,
     )
     end_log_group()
 

cpp_linter/clang_tools/clang_format.py

@@ -181,6 +181,7 @@ def run_clang_format(
     )
     if format_review:
         del cmds[2]  # remove `--output-replacements-xml` flag
+        logger.info('Getting fixes with "%s"', " ".join(cmds))
         # get formatted file from stdout
         formatted_output = subprocess.run(cmds, capture_output=True, check=True)
         # store formatted_output (for comparing later)

cpp_linter/clang_tools/clang_tidy.py

@@ -164,7 +164,7 @@ def run_clang_tidy(
         extra_args = extra_args[0].split()
     for extra_arg in extra_args:
         arg = extra_arg.strip('"')
-        cmds.append(f'--extra-arg="{arg}"')
+        cmds.append(f'--extra-arg={arg}')
     cmds.append(filename)
     logger.info('Running "%s"', " ".join(cmds))
     results = subprocess.run(cmds, capture_output=True)
@@ -182,10 +182,11 @@ def run_clang_tidy(
         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
+        logger.info('Getting fixes with "%s"', " ".join(cmds))
         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)
+        # re-write original file contents
         Path(file_obj.name).write_bytes(original_buf)
     return advice
 

cpp_linter/cli.py

@@ -287,15 +287,19 @@
     default="false",
     type=lambda input: input.lower() == "true",
     help="""Set to ``true`` to enable PR review suggestions
-from clang-tidy.""",
+from clang-tidy.
+
+Defaults to ``%(default)s``.""",
 )
 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.""",
+from clang-format.
+
+Defaults to ``%(default)s``.""",
 )
 
 

cpp_linter/rest_api/github_api.py

@@ -174,7 +174,9 @@ def post_feedback(
                 )
 
         if self.event_name == "pull_request" and (tidy_review or format_review):
-            self.post_review(files, tidy_advice, format_advice)
+            self.post_review(
+                files, tidy_advice, format_advice, tidy_review, format_review
+            )
 
         if file_annotations:
             self.make_annotations(files, format_advice, tidy_advice, style)
@@ -354,6 +356,8 @@ def post_review(
         files: List[FileObj],
         tidy_advice: List[TidyAdvice],
         format_advice: List[FormatAdvice],
+        tidy_review: bool,
+        format_review: bool,
     ):
         url = f"{self.api_url}/repos/{self.repo}/pulls/{self.event_payload['number']}"
         response_buffer = self.session.get(url, headers=self.make_headers())
@@ -372,26 +376,27 @@ def post_review(
         body = f"{COMMENT_MARKER}## Cpp-linter Review\n"
         payload_comments = []
         total_changes = 0
-        for index, tool_advice in enumerate([format_advice, tidy_advice]):
-            comments, total, patch = self.create_review_comments(
-                files,
-                tool_advice,  # type: ignore[arg-type]
-            )
-            tool = "clang-tidy" if index else "clang-format"
+        advice: Dict[str, Sequence[Union[TidyAdvice, FormatAdvice]]] = {}
+        if format_review:
+            advice["clang-format"] = format_advice
+        if tidy_review:
+            advice["clang-tidy"] = tidy_advice
+        for tool_name, tool_advice in advice.items():
+            comments, total, patch = self.create_review_comments(files, tool_advice)
             total_changes += total
             payload_comments.extend(comments)
             if total and total != len(comments):
-                body += f"Only {len(comments)} out of {total} {tool} "
+                body += f"Only {len(comments)} out of {total} {tool_name} "
                 body += "suggestions fit within this pull request's diff.\n"
             if patch:
-                body += f"\n<details><summary>Click here for the full {tool} patch"
+                body += f"\n<details><summary>Click here for the full {tool_name} patch"
                 body += f"</summary>\n\n\n```diff\n{patch}\n```\n\n\n</details>\n\n"
             else:
-                body += f"No objections from {tool}.\n"
+                body += f"No objections from {tool_name}.\n"
         if total_changes:
             event = "REQUEST_CHANGES"
         else:
-            body += "\nGreat job!"
+            body += "\nGreat job! :tada:"
             event = "APPROVE"
         body += USER_OUTREACH
         payload = {
@@ -414,8 +419,7 @@ def create_review_comments(
         comments = []
         full_patch = ""
         for file, advice in zip(files, tool_advice):
-            if not advice.patched:
-                continue
+            assert advice.patched, f"No suggested patch found for {file.name}"
             patch = Patch.create_from(
                 old=Path(file.name).read_bytes(),
                 new=advice.patched,

tests/capture_tools_output/test_tools_output.py

@@ -486,4 +486,4 @@ def test_tidy_extra_args(caplog: pytest.LogCaptureFixture, user_input: List[str]
     if len(user_input) == 1 and " " in user_input[0]:
         user_input = user_input[0].split()
     for a in user_input:
-        assert f'--extra-arg="{a}"' in messages[0]
+        assert f'--extra-arg={a}' in messages[0]

tests/test_cli_args.py

@@ -75,6 +75,8 @@ def test_defaults():
         ("file-annotations", "False", "file_annotations", False),
         ("extra-arg", "-std=c++17", "extra_arg", ["-std=c++17"]),
         ("extra-arg", '"-std=c++17 -Wall"', "extra_arg", ['"-std=c++17 -Wall"']),
+        ("tidy-review", "true", "tidy_review", True),
+        ("format-review", "true", "format_review", True),
     ],
 )
 def test_arg_parser(