From a938f2380727b5759beb8bbda1de7e8279f97466 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 4 Oct 2024 15:57:51 -0700 Subject: [PATCH 1/4] feat: capture and output clang version used --- cpp_linter/__init__.py | 4 +- cpp_linter/clang_tools/__init__.py | 41 ++++++++++++++----- cpp_linter/rest_api/__init__.py | 16 +++++++- cpp_linter/rest_api/github_api.py | 4 ++ .../test_database_path.py | 3 +- .../capture_tools_output/test_tools_output.py | 6 ++- tests/comments/test_comments.py | 4 +- tests/reviews/test_pr_review.py | 4 +- tests/test_comment_length.py | 5 +++ 9 files changed, 66 insertions(+), 21 deletions(-) diff --git a/cpp_linter/__init__.py b/cpp_linter/__init__.py index a78c3178..0cac86f2 100644 --- a/cpp_linter/__init__.py +++ b/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() diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index be7bb339..ff797f19 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/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,7 +77,30 @@ 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+)", re.MULTILINE) + + +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) + output = version_out.stdout.decode() + matched = VERSION_PATTERN.search(output) + if matched is None: + raise RuntimeError( + f"Failed to get version numbers from `{cmd} --version` output" + ) + ver = cast(str, matched.group(1)) + logger.info("%s --version\n%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`. @@ -85,18 +108,13 @@ def capture_clang_tools_output(files: List[FileObj], args: Args): :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) + clang_versions.format = _capture_tool_version(format_cmd) tidy_filter = TidyFileFilter( extensions=args.extensions, ignore_value=args.ignore_tidy, @@ -105,7 +123,7 @@ def show_tool_version_output(cmd: str): # show version output for executable us # 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) + clang_versions.tidy = _capture_tool_version(tidy_cmd) format_filter = FormatFileFilter( extensions=args.extensions, ignore_value=args.ignore_format, @@ -155,3 +173,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 diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py index fb72230f..5c9f60a0 100644 --- a/cpp_linter/rest_api/__init__.py +++ b/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
clang-format reports: " + comment = "\n
clang-format{} reports: ".format( + "" if version is None else f" (v{version})" + ) comment += f"{checks_failed} file(s) not formatted\n\n" closer = "\n
" 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
clang-tidy reports: " + comment = "\n
clang-tidy{} reports: ".format( + "" if version is None else f" (v{version})" + ) comment += f"{checks_failed} concern(s)\n\n" closer = "\n
" for file_obj in files: @@ -276,6 +287,7 @@ def post_feedback( self, files: List[FileObj], args: Args, + clang_versions: ClangVersions, ): """Post action's results using REST API. diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index a6e922b1..5595a972 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/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, ) diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py index 3a0e685f..8af87dda 100644 --- a/tests/capture_tools_output/test_database_path.py +++ b/tests/capture_tools_output/test_database_path.py @@ -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 = 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 diff --git a/tests/capture_tools_output/test_tools_output.py b/tests/capture_tools_output/test_tools_output.py index 03efd326..d674088a 100644 --- a/tests/capture_tools_output/test_tools_output.py +++ b/tests/capture_tools_output/test_tools_output.py @@ -16,7 +16,7 @@ from cpp_linter.common_fs import FileObj, CACHE_PATH from cpp_linter.git import parse_diff, get_diff -from cpp_linter.clang_tools import capture_clang_tools_output +from cpp_linter.clang_tools import capture_clang_tools_output, ClangVersions 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.loggers import log_commander, logger @@ -67,10 +67,14 @@ def make_comment( ): format_checks_failed = tally_format_advice(files) tidy_checks_failed = tally_tidy_advice(files) + clang_versions = ClangVersions() + clang_versions.format = "x.y.z" + clang_versions.tidy = "x.y.z" comment = GithubApiClient.make_comment( files=files, tidy_checks_failed=tidy_checks_failed, format_checks_failed=format_checks_failed, + clang_versions=clang_versions, ) return comment, format_checks_failed, tidy_checks_failed diff --git a/tests/comments/test_comments.py b/tests/comments/test_comments.py index 86cf7954..a8ab65c5 100644 --- a/tests/comments/test_comments.py +++ b/tests/comments/test_comments.py @@ -66,7 +66,7 @@ def test_post_feedback( args.thread_comments = thread_comments args.step_summary = thread_comments == "update" and not no_lgtm args.file_annotations = thread_comments == "update" and no_lgtm - capture_clang_tools_output(files, args=args) + clang_versions = capture_clang_tools_output(files, args=args) # add a non project file to tidy_advice to intentionally cover a log.debug() for file in files: if file.tidy_advice: @@ -169,4 +169,4 @@ def test_post_feedback( # to get debug files saved to test workspace folders: enable logger verbosity caplog.set_level(logging.DEBUG, logger=logger.name) - gh_client.post_feedback(files, args) + gh_client.post_feedback(files, args, clang_versions) diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 3f7de7aa..50a1fc0a 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -170,7 +170,7 @@ def test_post_review( args.file_annotations = False args.passive_reviews = is_passive - capture_clang_tools_output(files, args=args) + clang_versions = capture_clang_tools_output(files, args=args) if not force_approved: format_advice = list(filter(lambda x: x.format_advice is not None, files)) tidy_advice = list(filter(lambda x: x.tidy_advice is not None, files)) @@ -194,7 +194,7 @@ def test_post_review( headers={"Accept": "application/vnd.github.text+json"}, text=cache_pr_response, ) - gh_client.post_feedback(files, args) + gh_client.post_feedback(files, args, clang_versions) # inspect the review payload for correctness last_request = mock.last_request diff --git a/tests/test_comment_length.py b/tests/test_comment_length.py index 47c98521..8414b56d 100644 --- a/tests/test_comment_length.py +++ b/tests/test_comment_length.py @@ -3,6 +3,7 @@ from cpp_linter.rest_api import USER_OUTREACH from cpp_linter.clang_tools.clang_format import FormatAdvice, FormatReplacementLine from cpp_linter.common_fs import FileObj +from cpp_linter.clang_tools import ClangVersions def test_comment_length_limit(tmp_path: Path): @@ -15,11 +16,14 @@ def test_comment_length_limit(tmp_path: Path): dummy_advice = FormatAdvice(file_name) dummy_advice.replaced_lines = [FormatReplacementLine(line_numb=1)] file.format_advice = dummy_advice + versions = ClangVersions() + versions.format = "x.y.z" files = [file] * format_checks_failed thread_comment = GithubApiClient.make_comment( files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, + clang_versions=versions, len_limit=abs_limit, ) assert len(thread_comment) < abs_limit @@ -28,6 +32,7 @@ def test_comment_length_limit(tmp_path: Path): files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, + clang_versions=versions, len_limit=None, ) assert len(step_summary) != len(thread_comment) From 5fa05c826bbb8f7425543882a83ce3a74ad0b790 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 4 Oct 2024 18:38:56 -0700 Subject: [PATCH 2/4] add version to PR review summary also fix some errors: 1. about filter files per clang tool 2. merged suggestions' file name must match exactly --- cpp_linter/clang_tools/__init__.py | 12 ++++++------ cpp_linter/clang_tools/clang_tidy.py | 5 ++++- cpp_linter/clang_tools/patcher.py | 19 +++++++++++++++++-- cpp_linter/rest_api/github_api.py | 8 +++++++- tests/reviews/test_pr_review.py | 19 +++++++++++-------- 5 files changed, 45 insertions(+), 18 deletions(-) diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index ff797f19..4563b60a 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/cpp_linter/clang_tools/__init__.py @@ -85,12 +85,12 @@ def _capture_tool_version(cmd: str) -> str: version_out = subprocess.run([cmd, "--version"], capture_output=True, check=True) output = version_out.stdout.decode() matched = VERSION_PATTERN.search(output) - if matched is None: + 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\n%s", cmd, ver) + logger.info("`%s --version`: %s", cmd, ver) return ver @@ -115,18 +115,18 @@ def capture_clang_tools_output(files: List[FileObj], args: Args) -> ClangVersion format_cmd = assemble_version_exec("clang-format", args.version) assert format_cmd is not None, "clang-format executable was not found" clang_versions.format = _capture_tool_version(format_cmd) - tidy_filter = TidyFileFilter( + 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" clang_versions.tidy = _capture_tool_version(tidy_cmd) - format_filter = FormatFileFilter( + tidy_filter = TidyFileFilter( extensions=args.extensions, - ignore_value=args.ignore_format, + ignore_value=args.ignore_tidy, ) db_json: Optional[List[Dict[str, str]]] = None diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index af4821ba..47acbcff 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -133,7 +133,10 @@ 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 diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index 7354f773..5e5e64af 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/cpp_linter/clang_tools/patcher.py @@ -69,17 +69,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_versions: The version numbers of the clang-tidy used. + :param format_versions: 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 +107,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: # if tool wasn't used + continue + summary += f"### Used {tool_name} v{tool_version}\n\n" if ( len(comments) and posted_tool_advice[tool_name] != self.tool_total[tool_name] diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 5595a972..e9380bdd 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -257,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( @@ -392,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) @@ -423,7 +425,11 @@ 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 diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 50a1fc0a..50b8b438 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -13,10 +13,6 @@ TEST_REPO = "cpp-linter/test-cpp-linter-action" TEST_PR = 27 -DEFAULT_TIDY_CHECKS = ( - "boost-*,bugprone-*,performance-*,readability-*,portability-*,modernize-*," - "clang-analyzer-*,cppcoreguidelines-*" -) test_parameters = OrderedDict( is_draft=False, @@ -155,9 +151,10 @@ def test_post_review( files.clear() args = Args() - args.tidy_checks = DEFAULT_TIDY_CHECKS + if not tidy_review: + args.tidy_checks = "-*" args.version = environ.get("CLANG_VERSION", "16") - args.style = "file" + args.style = "file" if format_review else "" args.extensions = extensions args.ignore_tidy = "*.c" args.ignore_format = "*.c" @@ -174,8 +171,14 @@ def test_post_review( if not force_approved: format_advice = list(filter(lambda x: x.format_advice is not None, files)) tidy_advice = list(filter(lambda x: x.tidy_advice is not None, files)) - assert tidy_advice and len(tidy_advice) < len(files) - assert format_advice and len(format_advice) < len(files) + if tidy_review: + assert tidy_advice and len(tidy_advice) <= len(files) + else: + assert not tidy_advice + if format_review: + assert format_advice and len(format_advice) <= len(files) + else: + assert not format_advice # simulate draft PR by changing the request response cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( From 4d67d414cbdf2406530e9846a4196488a70557ca Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 4 Oct 2024 21:53:37 -0700 Subject: [PATCH 3/4] better tool summary exclusion from review summary when tool was not used at all OR review was not requested from a particular tool --- cpp_linter/clang_tools/clang_tidy.py | 1 + cpp_linter/clang_tools/patcher.py | 19 +++++++++++++------ cpp_linter/rest_api/github_api.py | 6 +++++- tests/reviews/test_pr_review.py | 7 ++----- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 47acbcff..d5c0f5bd 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -142,6 +142,7 @@ def _has_related_suggestion(suggestion: Suggestion) -> bool: 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) diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index 5e5e64af..6fa61a8a 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/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": ""} @@ -110,8 +115,8 @@ def serialize_to_github_payload( tool_version = tidy_version if tool_name == "clang-format": tool_version = format_version - if tool_version is None: # if tool wasn't used - continue + 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) @@ -130,8 +135,7 @@ def serialize_to_github_payload( ) 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): @@ -179,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) @@ -208,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 diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index e9380bdd..c6c09d4d 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -433,7 +433,7 @@ def post_review( 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: @@ -467,6 +467,10 @@ def create_review_comments( :param review_comments: An object (passed by reference) that is used to store the results. """ + if tidy_tool: + review_comments.tool_total["clang-tidy"] = 0 + else: + review_comments.tool_total["clang-format"] = 0 for file_obj in files: tool_advice: Optional[PatchMixin] if tidy_tool: diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 50b8b438..1d1aaf19 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -154,7 +154,7 @@ def test_post_review( if not tidy_review: args.tidy_checks = "-*" args.version = environ.get("CLANG_VERSION", "16") - args.style = "file" if format_review else "" + args.style = "file" args.extensions = extensions args.ignore_tidy = "*.c" args.ignore_format = "*.c" @@ -175,10 +175,7 @@ def test_post_review( assert tidy_advice and len(tidy_advice) <= len(files) else: assert not tidy_advice - if format_review: - assert format_advice and len(format_advice) <= len(files) - else: - assert not format_advice + assert format_advice and len(format_advice) <= len(files) # simulate draft PR by changing the request response cache_pr_response = (cache_path / f"pr_{TEST_PR}.json").read_text( From ddb10c885a2be20d26184fc21a41fe075d0973ba Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 4 Oct 2024 23:38:55 -0700 Subject: [PATCH 4/4] review changes --- cpp_linter/clang_tools/__init__.py | 15 +++++++++------ cpp_linter/clang_tools/patcher.py | 4 ++-- cpp_linter/rest_api/__init__.py | 1 + cpp_linter/rest_api/github_api.py | 6 ++---- tests/capture_tools_output/test_database_path.py | 4 ++-- tests/test_comment_length.py | 8 ++++---- 6 files changed, 20 insertions(+), 18 deletions(-) diff --git a/cpp_linter/clang_tools/__init__.py b/cpp_linter/clang_tools/__init__.py index 4563b60a..b762cdf8 100644 --- a/cpp_linter/clang_tools/__init__.py +++ b/cpp_linter/clang_tools/__init__.py @@ -77,14 +77,15 @@ def _run_on_single_file( return file.name, log_stream.getvalue(), tidy_note, format_advice -VERSION_PATTERN = re.compile(r"version\s(\d+\.\d+\.\d+)", re.MULTILINE) +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) - output = version_out.stdout.decode() - matched = VERSION_PATTERN.search(output) + 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" @@ -113,7 +114,8 @@ def capture_clang_tools_output(files: List[FileObj], args: Args) -> ClangVersion 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" + 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, @@ -122,7 +124,8 @@ def capture_clang_tools_output(files: List[FileObj], args: Args) -> ClangVersion 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" + 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, diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index 6fa61a8a..5b99cee3 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/cpp_linter/clang_tools/patcher.py @@ -91,8 +91,8 @@ def serialize_to_github_payload( """Serialize this object into a summary and list of comments compatible with Github's REST API. - :param tidy_versions: The version numbers of the clang-tidy used. - :param format_versions: The version numbers of the clang-format used. + :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 diff --git a/cpp_linter/rest_api/__init__.py b/cpp_linter/rest_api/__init__.py index 5c9f60a0..c6bfe331 100644 --- a/cpp_linter/rest_api/__init__.py +++ b/cpp_linter/rest_api/__init__.py @@ -293,6 +293,7 @@ def post_feedback( :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") diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index c6c09d4d..b48ddd39 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -467,10 +467,8 @@ def create_review_comments( :param review_comments: An object (passed by reference) that is used to store the results. """ - if tidy_tool: - review_comments.tool_total["clang-tidy"] = 0 - else: - review_comments.tool_total["clang-format"] = 0 + 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: diff --git a/tests/capture_tools_output/test_database_path.py b/tests/capture_tools_output/test_database_path.py index 8af87dda..c468b015 100644 --- a/tests/capture_tools_output/test_database_path.py +++ b/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 - clang_versions = 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: diff --git a/tests/test_comment_length.py b/tests/test_comment_length.py index 8414b56d..0d4359c6 100644 --- a/tests/test_comment_length.py +++ b/tests/test_comment_length.py @@ -16,14 +16,14 @@ def test_comment_length_limit(tmp_path: Path): dummy_advice = FormatAdvice(file_name) dummy_advice.replaced_lines = [FormatReplacementLine(line_numb=1)] file.format_advice = dummy_advice - versions = ClangVersions() - versions.format = "x.y.z" + clang_versions = ClangVersions() + clang_versions.format = "x.y.z" files = [file] * format_checks_failed thread_comment = GithubApiClient.make_comment( files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, - clang_versions=versions, + clang_versions=clang_versions, len_limit=abs_limit, ) assert len(thread_comment) < abs_limit @@ -32,7 +32,7 @@ def test_comment_length_limit(tmp_path: Path): files=files, format_checks_failed=format_checks_failed, tidy_checks_failed=0, - clang_versions=versions, + clang_versions=clang_versions, len_limit=None, ) assert len(step_summary) != len(thread_comment)