diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index 08b7526..ac93ade 100644 --- a/cpp_linter/__init__.py +++ b/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() diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 193e309..f6888b7 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/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) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 8c1eca6..5887b51 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/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 diff --git a/cpp_linter/cli.py b/cpp_linter/cli.py index 28b8605..3a56245 100644 --- a/cpp_linter/cli.py +++ b/cpp_linter/cli.py @@ -287,7 +287,9 @@ 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", @@ -295,7 +297,9 @@ 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``.""", ) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 8aed395..eee21b6 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/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
Click here for the full {tool} patch" + body += f"\n
Click here for the full {tool_name} patch" body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\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, diff --git a/tests/capture_tools_output/test_tools_output.py b/tests/capture_tools_output/test_tools_output.py index 5f1997f..ee21cfe 100644 --- a/tests/capture_tools_output/test_tools_output.py +++ b/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] diff --git a/tests/test_cli_args.py b/tests/test_cli_args.py index 6376497..f67a90e 100644 --- a/tests/test_cli_args.py +++ b/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(