From ab270f5c860b7e016583eaac560da68fbf91310d Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 10 Feb 2024 01:45:45 -0800 Subject: [PATCH 1/7] resolves #65 This will remove the line "No objections from ..." when it is not relevant in PR review summary comments. Also adds a :tada: for approved PR reviews --- cpp_linter/rest_api/github_api.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 8aed395..46d7c86 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()) @@ -386,12 +390,13 @@ def post_review( if patch: body += f"\n
Click here for the full {tool} patch" body += f"\n\n\n```diff\n{patch}\n```\n\n\n
\n\n" - else: + elif (index and tidy_review) or (not index and format_review): + # only include this line if it is relevant. body += f"No objections from {tool}.\n" if total_changes: event = "REQUEST_CHANGES" else: - body += "\nGreat job!" + body += "\nGreat job! :tada:" event = "APPROVE" body += USER_OUTREACH payload = { From 1f69aff659f4d5b44e051ad9569470048d34714c Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 10 Feb 2024 15:49:54 -0800 Subject: [PATCH 2/7] change logic about traversing advice for different tools --- cpp_linter/clang_tools/clang_tidy.py | 2 +- cpp_linter/rest_api/github_api.py | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 8c1eca6..ac476be 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -185,7 +185,7 @@ def run_clang_tidy( 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/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 46d7c86..bc93ba5 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -376,23 +376,26 @@ 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]): + 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, # type: ignore[arg-type] ) - tool = "clang-tidy" if index else "clang-format" 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" - elif (index and tidy_review) or (not index and format_review): - # only include this line if it is relevant. - body += f"No objections from {tool}.\n" + else: + body += f"No objections from {tool_name}.\n" if total_changes: event = "REQUEST_CHANGES" else: @@ -419,8 +422,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, From 2a59f2291568f733b7fc93545ad8b384c94c67a1 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 11 Feb 2024 11:37:00 -0800 Subject: [PATCH 3/7] show cmd used to get fixes in logs --- cpp_linter/clang_tools/clang_format.py | 1 + cpp_linter/clang_tools/clang_tidy.py | 1 + 2 files changed, 2 insertions(+) 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 ac476be..9200bd6 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -182,6 +182,7 @@ 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() From b2bbc7b85200521de0de07cc88a746c10ff260a7 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 11 Feb 2024 11:52:30 -0800 Subject: [PATCH 4/7] don't add quotes around extra-arg value --- cpp_linter/clang_tools/clang_tidy.py | 2 +- tests/capture_tools_output/test_tools_output.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 9200bd6..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) 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] From 261317d919739ffea8433b20f92ec5095c7184f3 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 11 Feb 2024 12:32:38 -0800 Subject: [PATCH 5/7] show me what advice is missing patch --- cpp_linter/cli.py | 8 ++++++-- cpp_linter/rest_api/github_api.py | 9 ++++----- tests/test_cli_args.py | 2 ++ 3 files changed, 12 insertions(+), 7 deletions(-) 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 bc93ba5..85d5500 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -382,10 +382,7 @@ def post_review( 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, # type: ignore[arg-type] - ) + comments, total, patch = self.create_review_comments(files, tool_advice) total_changes += total payload_comments.extend(comments) if total and total != len(comments): @@ -422,7 +419,9 @@ def create_review_comments( comments = [] full_patch = "" for file, advice in zip(files, tool_advice): - assert advice.patched, f"No suggested patch found for {file.name}" + assert ( + advice.patched + ), f"No suggested patch found for {file.name} in {type(advice)}" patch = Patch.create_from( old=Path(file.name).read_bytes(), new=advice.patched, 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( From 391825c0013c2f0730cf6a0a38208796de17ac44 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 11 Feb 2024 12:44:20 -0800 Subject: [PATCH 6/7] correct order of arg to post_feedback() use kwargs to avoid mis-ordering positional args --- cpp_linter/__init__.py | 52 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-) 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() From f464dcbb4bc2441a0980451729a40908117a0c4d Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sun, 11 Feb 2024 13:15:30 -0800 Subject: [PATCH 7/7] clean up some debugging output --- cpp_linter/rest_api/github_api.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 85d5500..eee21b6 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -419,9 +419,7 @@ def create_review_comments( comments = [] full_patch = "" for file, advice in zip(files, tool_advice): - assert ( - advice.patched - ), f"No suggested patch found for {file.name} in {type(advice)}" + assert advice.patched, f"No suggested patch found for {file.name}" patch = Patch.create_from( old=Path(file.name).read_bytes(), new=advice.patched,