From 7c47b686c0d70628cac298429d3b0b03cdfb38d8 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 3 Sep 2024 22:55:02 -0700 Subject: [PATCH 01/12] test with clang tools v18 include hidden files in artifact upload --- .github/workflows/run-dev-tests.yml | 2 +- tests/demo/.clang-tidy | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/run-dev-tests.yml b/.github/workflows/run-dev-tests.yml index 0704ae95..b7022ecd 100644 --- a/.github/workflows/run-dev-tests.yml +++ b/.github/workflows/run-dev-tests.yml @@ -43,7 +43,7 @@ jobs: matrix: py: ['3.8', '3.9', '3.10', '3.11'] os: ['windows-latest', ubuntu-22.04] - version: ['17', '16', '15', '14', '13', '12', '11', '10', '9', '8', '7'] + version: ['18', '17', '16', '15', '14', '13', '12', '11', '10', '9', '8', '7'] runs-on: ${{ matrix.os }} steps: diff --git a/tests/demo/.clang-tidy b/tests/demo/.clang-tidy index d3865ade..ba113044 100644 --- a/tests/demo/.clang-tidy +++ b/tests/demo/.clang-tidy @@ -2,7 +2,6 @@ Checks: 'clang-diagnostic-*,clang-analyzer-*,-*,performance-*,bugprone-*,clang-analyzer-*,mpi-*,misc-*,readability-*' WarningsAsErrors: '' HeaderFilterRegex: '' -AnalyzeTemporaryDtors: false FormatStyle: 'file' CheckOptions: - key: bugprone-argument-comment.CommentBoolLiterals From 68953fbd368388b596a583e69f49a4299f424c29 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 3 Sep 2024 19:35:18 -0700 Subject: [PATCH 02/12] pyproject.toml use deps from requirements.txt - show slowest tests - force use colored pytest output (for CI logs) --- pyproject.toml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b1f7f673..8a4c1622 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,11 +12,6 @@ authors = [ { name = "Brendan Doherty", email = "2bndy5@gmail.com" }, { name = "Peter Shen", email = "xianpeng.shen@gmail.com" }, ] -dependencies = [ - "requests", - "pyyaml", - "pygit2", -] classifiers = [ # https://pypi.org/pypi?%3Aaction=list_classifiers "Development Status :: 5 - Production/Stable", @@ -31,7 +26,7 @@ classifiers = [ "Programming Language :: Python :: 3", "Topic :: Software Development :: Build Tools", ] -dynamic = ["version"] +dynamic = ["version", "dependencies"] [project.scripts] cpp-linter = "cpp_linter:main" @@ -47,6 +42,9 @@ tracker = "https://github.com/cpp-linter/cpp-linter/issues" zip-safe = false packages = ["cpp_linter"] +[tool.setuptools.dynamic] +dependencies = {file = ["requirements.txt"]} + [tool.setuptools_scm] # It would be nice to include the commit hash in the version, but that # can't be done in a PEP 440-compatible way. @@ -61,7 +59,7 @@ show_column_numbers = true [tool.pytest.ini_options] minversion = "6.0" -addopts = "-vv" +addopts = "-vv --durations=8 --color=yes" testpaths = ["tests"] [tool.coverage] From 033d7a39754da3b988dc526b0a6312c9639f6209 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 4 Sep 2024 01:00:31 -0700 Subject: [PATCH 03/12] create cspell config --- cspell.config.yml | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 cspell.config.yml diff --git a/cspell.config.yml b/cspell.config.yml new file mode 100644 index 00000000..db8552e6 --- /dev/null +++ b/cspell.config.yml @@ -0,0 +1,43 @@ +version: "0.2" +language: en +words: + - argnames + - argvalues + - automodule + - bugprone + - caplog + - codecov + - codespell + - consts + - cppcoreguidelines + - cstdio + - docutils + - endgroup + - Fixit + - gitmodules + - iomanip + - keepends + - libgit + - markdownlint + - maxsplit + - posix + - pybind + - pygit + - ratelimit + - revparse + - setenv + - tada + - toctree + - tofile + - tomli + - unidiff + - vararg + - venv +ignorePaths: + - .env/** + - .venv/** + - env/** + - venv/** + - tests/**/*.json + - '**.clang-tidy' + - '**.clang-format' From 6f04648a1d0fce6a44781502484b38b06fefbfa8 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 5 Sep 2024 00:46:54 -0700 Subject: [PATCH 04/12] abstract making suggestions out from GithubApiClient --- cpp_linter/clang_tools/clang_format.py | 11 +- cpp_linter/clang_tools/clang_tidy.py | 36 +++- cpp_linter/clang_tools/patcher.py | 161 ++++++++++++++++++ cpp_linter/common_fs/__init__.py | 3 + cpp_linter/rest_api/github_api.py | 136 ++++----------- .../cpp_linter.clang_tools.patcher.rst | 5 + docs/index.rst | 1 + tests/reviews/test_pr_review.py | 2 +- 8 files changed, 242 insertions(+), 113 deletions(-) create mode 100644 cpp_linter/clang_tools/patcher.py create mode 100644 docs/API-Reference/cpp_linter.clang_tools.patcher.rst diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index 913cf300..e9e2b6a5 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -2,12 +2,13 @@ from pathlib import PurePath import subprocess -from typing import List, cast, Optional +from typing import List, cast import xml.etree.ElementTree as ET from ..common_fs import get_line_cnt_from_cols, FileObj from ..loggers import logger +from .patcher import PatchMixin class FormatReplacement: @@ -53,7 +54,7 @@ def __repr__(self): ) -class FormatAdvice: +class FormatAdvice(PatchMixin): """A single object to represent each suggestion. :param filename: The source file's name for which the contents of the xml @@ -69,8 +70,7 @@ 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 + super().__init__() def __repr__(self) -> str: return ( @@ -78,6 +78,9 @@ def __repr__(self) -> str: f"replacements for {self.filename}>" ) + def get_suggestion_help(self, start, end) -> str: + return "### clang-format suggestion\n" + def tally_format_advice(files: List[FileObj]) -> int: """Returns the sum of clang-format errors""" diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 48b156c4..6c9b8138 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -8,6 +8,7 @@ from typing import Tuple, Union, List, cast, Optional, Dict, Set from ..loggers import logger from ..common_fs import FileObj +from .patcher import PatchMixin, ReviewComments, Suggestion NOTE_HEADER = re.compile(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$") FIXED_NOTE = re.compile(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$") @@ -98,10 +99,10 @@ def __repr__(self) -> str: ) -class TidyAdvice: +class TidyAdvice(PatchMixin): def __init__(self, notes: List[TidyNotification]) -> None: #: A buffer of the applied fixes from clang-tidy - self.patched: Optional[bytes] = None + super().__init__() self.notes = notes def diagnostics_in_range(self, start: int, end: int) -> str: @@ -115,6 +116,37 @@ def diagnostics_in_range(self, start: int, end: int) -> str: break return diagnostics + def get_suggestion_help(self, start: int, end: int) -> str: + diagnostics = self.diagnostics_in_range(start, end) + if diagnostics: + return "### clang-tidy diagnostics\n" + diagnostics + return "### clang-tidy suggestion\n" + + def get_suggestions_from_patch( + self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments + ): + super().get_suggestions_from_patch(file_obj, summary_only, review_comments) + # now check for clang-tidy warnings with no fixes applied + for note in self.notes: + if not note.applied_fixes: # if no fix was applied + review_comments.total += 1 + line_numb = int(note.line) + if not summary_only and file_obj.is_range_contained( + start=line_numb, end=line_numb + 1 + ): + suggestion = Suggestion(file_obj.name) + suggestion.line_end = line_numb + body = f"### clang-tidy diagnostic\n**{file_obj.name}:" + body += f"{note.line}:{note.cols}:** {note.severity}: " + body += f"[{note.diagnostic_link}]\n> {note.rationale}\n" + if note.fixit_lines: + body += f'```{Path(file_obj.name).suffix.lstrip(".")}\n' + for fixit_line in note.fixit_lines: + body += f"{fixit_line}\n" + body += "```\n" + suggestion.comment = body + review_comments.suggestions.append(suggestion) + def tally_tidy_advice(files: List[FileObj]) -> int: """Returns the sum of clang-format errors""" diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py new file mode 100644 index 00000000..e57cf996 --- /dev/null +++ b/cpp_linter/clang_tools/patcher.py @@ -0,0 +1,161 @@ +"""A module to contain the abstractions about creating suggestions from a diff generated +by the clang tool's output.""" + +from abc import ABC +from pathlib import Path +from typing import Optional, Dict, Any, List, Tuple +from pygit2 import Patch # type: ignore +from ..common_fs import FileObj + +try: + from pygit2.enums import DiffOption # type: ignore + + INDENT_HEURISTIC = DiffOption.INDENT_HEURISTIC +except ImportError: # if pygit2.__version__ < 1.14 + from pygit2 import GIT_DIFF_INDENT_HEURISTIC # type: ignore + + INDENT_HEURISTIC = GIT_DIFF_INDENT_HEURISTIC + + +class Suggestion: + """A data structure to contain information about a single suggestion. + + :param file_name: The path to the file that this suggestion pertains. + This should use posix path separators. + """ + + def __init__(self, file_name: str) -> None: + #: The file's line number starting the suggested change. + self.line_start: int = -1 + #: The file's line number ending the suggested change. + self.line_end: int = -1 + #: The file's path about the suggested change. + self.file_name: str = file_name + #: The markdown comment about the suggestion. + self.comment: str = "" + + def serialize_to_github_payload(self) -> Dict[str, Any]: + """Serialize this object into a JSON compatible with Github's REST API.""" + assert self.line_end > 0, "ending line number unknown" + result = {"path": self.file_name, "body": self.comment, "line": self.line_end} + if self.line_start != self.line_end and self.line_start > 0: + result["start_line"] = self.line_start + return result + + +class ReviewComments: + """A data structure to contain PR review comments from a specific clang tool.""" + + def __init__(self) -> None: + #: The list of actual comments + self.suggestions: List[Suggestion] = [] + + self.total: int = 0 + """The total number of concerns. + + This may not equate to the length of `suggestions` because there is no + guarantee that all suggestions will fit within the PR's diff.""" + + self.full_patch: str = "" + """The full patch of all the suggestions (including those that will not + fit within the diff)""" + + def serialize_to_github_payload( + self, tool_name: str + ) -> Tuple[str, List[Dict[str, Any]]]: + """Serialize this object into a summary and list of comments compatible + with Github's REST API. + + :param tool_name: The clang tool's name that generated the suggestions. + + :returns: The returned tuple contains a brief summary (at index ``0``) + that contains markdown text describing the summary of the review + comments. + + The list of `suggestions` (at index ``1``) is the serialized JSON + object. + """ + len_suggestions = len(self.suggestions) + summary = "" + comments = [] + if len_suggestions: + comments = [x.serialize_to_github_payload() for x in self.suggestions] + if self.total and self.total != len_suggestions: + summary += f"Only {len_suggestions} out of {self.total} {tool_name} " + summary += "concerns fit within this pull request's diff.\n" + if self.full_patch: + summary += f"\n
Click here for the full {tool_name} patch" + summary += ( + f"\n\n\n```diff\n{self.full_patch}\n```\n\n\n
\n\n" + ) + elif not self.total: + summary += f"No concerns from {tool_name}.\n" + result = (summary, comments) + return result + + +class PatchMixin(ABC): + """An abstract mixin that unified parsing of the suggestions into + PR review comments.""" + + def __init__(self) -> None: + #: A unified diff of the applied fixes from the clang tool's output + self.patched: Optional[bytes] = None + + def get_suggestion_help(self, start, end) -> str: + """Create helpful text about what the suggestion aims to fix. + + The parameters ``start`` and ``end`` are the line numbers (relative to file's + original content) encapsulating the suggestion. + """ + + raise NotImplementedError("derivative must implement this abstract method") + + def get_suggestions_from_patch( + self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments + ): + """Create a list of suggestions from the tool's `patched` output. + + Results are stored in the ``review_comments`` parameter (passed by reference). + """ + + assert ( + self.patched + ), f"{self.__class__.__name__} has no suggestions for {file_obj.name}" + patch = Patch.create_from( + Path(file_obj.name).read_bytes(), + self.patched, + file_obj.name, + file_obj.name, + context_lines=0, # exclude any surrounding unchanged lines + flag=INDENT_HEURISTIC, + ) + review_comments.full_patch += f"{patch.text}" + for hunk in patch.hunks: + review_comments.total += 1 + if summary_only: + continue + new_hunk_range = file_obj.is_hunk_contained(hunk) + if new_hunk_range is None: + continue + start_line, end_line = new_hunk_range + comment = Suggestion(file_obj.name) + body = self.get_suggestion_help(start=start_line, end=end_line) + if start_line < end_line: + comment.line_start = start_line + comment.line_end = end_line + removed = [] + suggestion = "" + for line in hunk.lines: + if line.origin in ("+", " "): + suggestion += f"{line.content}" + else: + line_numb = line.new_lineno + removed.append(line_numb) + if not suggestion and removed: + body += "\nPlease remove the line(s)\n- " + body += "\n- ".join([str(x) for x in removed]) + else: + body += f"\n```suggestion\n{suggestion}```" + comment.comment = body + review_comments.suggestions.append(comment) diff --git a/cpp_linter/common_fs/__init__.py b/cpp_linter/common_fs/__init__.py index 9199ce49..974b8dd6 100644 --- a/cpp_linter/common_fs/__init__.py +++ b/cpp_linter/common_fs/__init__.py @@ -48,6 +48,9 @@ def __init__( #: The results from clang-format self.format_advice: Optional["FormatAdvice"] = None + def __repr__(self) -> str: + return f"" + @staticmethod def _consolidate_list_to_ranges(numbers: List[int]) -> List[List[int]]: """A helper function that is only used after parsing the lines from a diff that diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 92731223..98d188a9 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -15,31 +15,21 @@ from pathlib import Path import urllib.parse import sys -from typing import Dict, List, Any, cast, Optional, Tuple, Union +from typing import Dict, List, Any, cast, Optional -from pygit2 import Patch # type: ignore from ..common_fs import FileObj, CACHE_PATH from ..common_fs.file_filter import FileFilter from ..clang_tools.clang_format import ( - FormatAdvice, formalize_style_name, tally_format_advice, ) -from ..clang_tools.clang_tidy import TidyAdvice, tally_tidy_advice +from ..clang_tools.clang_tidy import tally_tidy_advice +from ..clang_tools.patcher import ReviewComments, PatchMixin from ..cli import Args from ..loggers import logger, log_commander from ..git import parse_diff, get_diff from . import RestApiClient, USER_OUTREACH, COMMENT_MARKER, RateLimitHeaders -try: - from pygit2.enums import DiffOption # type: ignore - - INDENT_HEURISTIC = DiffOption.INDENT_HEURISTIC -except ImportError: # if pygit2.__version__ < 1.14 - from pygit2 import GIT_DIFF_INDENT_HEURISTIC # type: ignore - - INDENT_HEURISTIC = GIT_DIFF_INDENT_HEURISTIC - RATE_LIMIT_HEADERS = RateLimitHeaders( reset="x-ratelimit-reset", remaining="x-ratelimit-remaining", @@ -417,26 +407,24 @@ def post_review( summary_only = ( environ.get("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false") == "true" ) - advice: Dict[str, bool] = {} + advice = [] if format_review: - advice["clang-format"] = False + advice.append("clang-format") if tidy_review: - advice["clang-tidy"] = True - for tool_name, tidy_tool in advice.items(): - comments, total, patch = self.create_review_comments( - files, tidy_tool, summary_only + advice.append("clang-tidy") + for tool_name in advice: + review_comments = ReviewComments() + self.create_review_comments( + files=files, + tidy_tool=tool_name == "clang-tidy", + summary_only=summary_only, + review_comments=review_comments, ) - total_changes += total + (summary, comments) = review_comments.serialize_to_github_payload(tool_name) if not summary_only: payload_comments.extend(comments) - if total and total != len(comments): - body += f"Only {len(comments)} out of {total} {tool_name} " - body += "concerns fit within this pull request's diff.\n" - if 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 not total: - body += f"No concerns from {tool_name}.\n" + body += summary + total_changes += review_comments.total if total_changes: event = "REQUEST_CHANGES" else: @@ -460,92 +448,28 @@ def create_review_comments( files: List[FileObj], tidy_tool: bool, summary_only: bool, - ) -> Tuple[List[Dict[str, Any]], int, str]: - """Creates a batch of comments for a specific clang tool's PR review""" - total = 0 - comments = [] - full_patch = "" + review_comments: ReviewComments, + ): + """Creates a batch of comments for a specific clang tool's PR review. + + :param files: The list of files to traverse. + :param tidy_tool: A flag to indicate if the suggestions should originate + from clang-tidy. + :param summary_only: A flag to indicate if only the review summary is desired. + :param review_comments: An object (passed by reference) that is used to store + the results. + """ for file_obj in files: - tool_advice: Optional[Union[TidyAdvice, FormatAdvice]] + tool_advice: Optional[PatchMixin] if tidy_tool: tool_advice = file_obj.tidy_advice else: tool_advice = file_obj.format_advice if not tool_advice: continue - assert tool_advice.patched, f"No suggested patch found for {file_obj.name}" - patch = Patch.create_from( - old=Path(file_obj.name).read_bytes(), - new=tool_advice.patched, - old_as_path=file_obj.name, - new_as_path=file_obj.name, - flag=INDENT_HEURISTIC, - context_lines=0, # trim all unchanged lines from start/end of hunks + tool_advice.get_suggestions_from_patch( + file_obj, summary_only, review_comments ) - full_patch += patch.text - for hunk in patch.hunks: - total += 1 - if summary_only: - continue - new_hunk_range = file_obj.is_hunk_contained(hunk) - if new_hunk_range is None: - continue - start_lines, end_lines = new_hunk_range - comment: Dict[str, Any] = {"path": file_obj.name} - body = "" - if tidy_tool and file_obj.tidy_advice: - body += "### clang-tidy " - diagnostics = file_obj.tidy_advice.diagnostics_in_range( - start_lines, end_lines - ) - if diagnostics: - body += "diagnostics\n" + diagnostics - else: - body += "suggestions\n" - elif not tidy_tool: - body += "### clang-format suggestions\n" - if start_lines < end_lines: - comment["start_line"] = start_lines - comment["line"] = end_lines - suggestion = "" - removed = [] - for line in hunk.lines: - if line.origin in ["+", " "]: - suggestion += line.content - else: - removed.append(line.old_lineno) - if not suggestion and removed: - body += "\nPlease remove the line(s)\n- " - body += "\n- ".join([str(x) for x in removed]) - else: - body += f"\n```suggestion\n{suggestion}```" - comment["body"] = body - comments.append(comment) - - # now check for clang-tidy warnings with no fixes applied - if tidy_tool and file_obj.tidy_advice: - for note in file_obj.tidy_advice.notes: - if not note.applied_fixes: # if no fix was applied - total += 1 - line_numb = int(note.line) - if file_obj.is_range_contained( - start=line_numb, end=line_numb + 1 - ): - diag: Dict[str, Any] = { - "path": file_obj.name, - "line": note.line, - } - body = f"### clang-tidy diagnostic\n**{file_obj.name}:" - body += f"{note.line}:{note.cols}:** {note.severity}: " - body += f"[{note.diagnostic_link}]\n> {note.rationale}\n" - if note.fixit_lines: - body += f'```{Path(file_obj.name).suffix.lstrip(".")}\n' - for line in note.fixit_lines: - body += f"{line}\n" - body += "```\n" - diag["body"] = body - comments.append(diag) - return (comments, total, full_patch) def _dismiss_stale_reviews(self, url: str): """Dismiss all reviews that were previously created by cpp-linter""" diff --git a/docs/API-Reference/cpp_linter.clang_tools.patcher.rst b/docs/API-Reference/cpp_linter.clang_tools.patcher.rst new file mode 100644 index 00000000..e7165b6c --- /dev/null +++ b/docs/API-Reference/cpp_linter.clang_tools.patcher.rst @@ -0,0 +1,5 @@ +``clang_tools.patcher`` +======================= + +.. automodule:: cpp_linter.clang_tools.patcher + :members: diff --git a/docs/index.rst b/docs/index.rst index b65a4815..e51602b1 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -16,6 +16,7 @@ API-Reference/cpp_linter.clang_tools API-Reference/cpp_linter.clang_tools.clang_format API-Reference/cpp_linter.clang_tools.clang_tidy + API-Reference/cpp_linter.clang_tools.patcher API-Reference/cpp_linter.rest_api API-Reference/cpp_linter.rest_api.github_api API-Reference/cpp_linter.git diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index 2853b8ff..d328b83e 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -123,7 +123,7 @@ def test_post_review( # load mock responses for pull_request event mock.get( base_url, - headers={"Accept": "application/vnd.github.diff"}, + request_headers={"Accept": "application/vnd.github.diff"}, text=(cache_path / f"pr_{TEST_PR}.diff").read_text(encoding="utf-8"), ) reviews = (cache_path / "pr_reviews.json").read_text(encoding="utf-8") From c34cd354bef1aa243542c8f359c80a0c51339716 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 5 Sep 2024 02:06:47 -0700 Subject: [PATCH 05/12] support more true-ish values for summary-only env var update doc --- cpp_linter/rest_api/github_api.py | 6 +++--- docs/pr_review_caveats.rst | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 98d188a9..466e00e9 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -404,9 +404,9 @@ def post_review( body = f"{COMMENT_MARKER}## Cpp-linter Review\n" payload_comments = [] total_changes = 0 - summary_only = ( - environ.get("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false") == "true" - ) + summary_only = environ.get( + "CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false" + ).lower() in ("true", "on", "1") advice = [] if format_review: advice.append("clang-format") diff --git a/docs/pr_review_caveats.rst b/docs/pr_review_caveats.rst index 77888bcc..fcf024b5 100644 --- a/docs/pr_review_caveats.rst +++ b/docs/pr_review_caveats.rst @@ -65,8 +65,8 @@ GitHub REST API does not provide a way to hide comments or mark review suggestio .. tip:: We do support an environment variable named ``CPP_LINTER_PR_REVIEW_SUMMARY_ONLY``. - If the variable is set to ``true``, then the review only contains a summary comment - with no suggestions posted in the diff. + If the variable is set either ``true``, ``on``, or ``1``, then the review only + contains a summary comment with no suggestions posted in the diff. Probable non-exhaustive reviews ------------------------------- From 8575f5c6be4a793795c5a6e4c86996c68a521313 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 5 Sep 2024 14:25:54 -0700 Subject: [PATCH 06/12] merge similar review comments add test --- cpp_linter/clang_tools/clang_format.py | 5 +- cpp_linter/clang_tools/clang_tidy.py | 21 ++++-- cpp_linter/clang_tools/patcher.py | 90 ++++++++++++++++++-------- cpp_linter/rest_api/github_api.py | 14 ++-- tests/reviews/test_pr_review.py | 2 + 5 files changed, 91 insertions(+), 41 deletions(-) diff --git a/cpp_linter/clang_tools/clang_format.py b/cpp_linter/clang_tools/clang_format.py index e9e2b6a5..a32139cf 100644 --- a/cpp_linter/clang_tools/clang_format.py +++ b/cpp_linter/clang_tools/clang_format.py @@ -79,7 +79,10 @@ def __repr__(self) -> str: ) def get_suggestion_help(self, start, end) -> str: - return "### clang-format suggestion\n" + return super().get_suggestion_help(start, end) + "suggestion\n" + + def get_tool_name(self) -> str: + return "clang-format" def tally_format_advice(files: List[FileObj]) -> int: diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 6c9b8138..51525572 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -118,18 +118,29 @@ def diagnostics_in_range(self, start: int, end: int) -> str: def get_suggestion_help(self, start: int, end: int) -> str: diagnostics = self.diagnostics_in_range(start, end) + prefix = super().get_suggestion_help(start, end) if diagnostics: - return "### clang-tidy diagnostics\n" + diagnostics - return "### clang-tidy suggestion\n" + return prefix + "diagnostics\n" + diagnostics + return prefix + "suggestion\n" + + def get_tool_name(self) -> str: + return "clang-tidy" def get_suggestions_from_patch( self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments ): super().get_suggestions_from_patch(file_obj, summary_only, review_comments) + + def _has_related_suggestion(suggestion: Suggestion) -> bool: + for known in review_comments.suggestions: + if known.line_end >= suggestion.line_end >= known.line_start: + known.comment += f"\n{suggestion.comment}" + return True + return False + # now check for clang-tidy warnings with no fixes applied for note in self.notes: if not note.applied_fixes: # if no fix was applied - review_comments.total += 1 line_numb = int(note.line) if not summary_only and file_obj.is_range_contained( start=line_numb, end=line_numb + 1 @@ -145,7 +156,9 @@ def get_suggestions_from_patch( body += f"{fixit_line}\n" body += "```\n" suggestion.comment = body - review_comments.suggestions.append(suggestion) + if not _has_related_suggestion(suggestion): + review_comments.tool_total["clang-tidy"] += 1 + review_comments.suggestions.append(suggestion) def tally_tidy_advice(files: List[FileObj]) -> int: diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index e57cf996..c92efeb2 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/cpp_linter/clang_tools/patcher.py @@ -50,24 +50,36 @@ def __init__(self) -> None: #: The list of actual comments self.suggestions: List[Suggestion] = [] - self.total: int = 0 - """The total number of concerns. + self.tool_total: Dict[str, int] = {"clang-tidy": 0, "clang-format": 0} + """The total number of concerns about a specific clang tool. - This may not equate to the length of `suggestions` because there is no - guarantee that all suggestions will fit within the PR's diff.""" + 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. + """ - self.full_patch: str = "" + self.full_patch: Dict[str, str] = {"clang-tidy": "", "clang-format": ""} """The full patch of all the suggestions (including those that will not fit within the diff)""" - def serialize_to_github_payload( - self, tool_name: str - ) -> Tuple[str, List[Dict[str, Any]]]: + def merge_similar_suggestion(self, suggestion: Suggestion) -> bool: + """Merge a given ``suggestion`` into a similar `Suggestion` + + :returns: `True` if the suggestion was merged, otherwise `False`. + """ + for known in self.suggestions: + if ( + 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]]]: """Serialize this object into a summary and list of comments compatible with Github's REST API. - :param tool_name: The clang tool's name that generated the suggestions. - :returns: The returned tuple contains a brief summary (at index ``0``) that contains markdown text describing the summary of the review comments. @@ -75,21 +87,34 @@ def serialize_to_github_payload( The list of `suggestions` (at index ``1``) is the serialized JSON object. """ - len_suggestions = len(self.suggestions) summary = "" comments = [] - if len_suggestions: - comments = [x.serialize_to_github_payload() for x in self.suggestions] - if self.total and self.total != len_suggestions: - summary += f"Only {len_suggestions} out of {self.total} {tool_name} " - summary += "concerns fit within this pull request's diff.\n" - if self.full_patch: - summary += f"\n
Click here for the full {tool_name} patch" - summary += ( - f"\n\n\n```diff\n{self.full_patch}\n```\n\n\n
\n\n" - ) - elif not self.total: - summary += f"No concerns from {tool_name}.\n" + posted_tool_advice = {"clang-tidy": 0, "clang-format": 0} + for comment in self.suggestions: + comments.append(comment.serialize_to_github_payload()) + if "### clang-format" in comment.comment: + posted_tool_advice["clang-format"] += 1 + if "### clang-tidy" in comment.comment: + posted_tool_advice["clang-tidy"] += 1 + + for tool_name in ("clang-tidy", "clang-format"): + if ( + len(comments) + and posted_tool_advice[tool_name] != self.tool_total[tool_name] + ): + summary += ( + f"Only {posted_tool_advice[tool_name]} out of " + + f"{self.tool_total[tool_name]} {tool_name}" + + " concerns fit within this pull request's diff.\n" + ) + if self.full_patch[tool_name]: + summary += ( + f"\n
Click here for the full {tool_name} patch" + + f"\n\n\n```diff\n{self.full_patch[tool_name]}\n" + + "```\n\n\n
\n\n" + ) + elif not self.tool_total[tool_name]: + summary += f"No concerns from {tool_name}.\n" result = (summary, comments) return result @@ -109,7 +134,13 @@ def get_suggestion_help(self, start, end) -> str: original content) encapsulating the suggestion. """ - raise NotImplementedError("derivative must implement this abstract method") + return f"### {self.get_tool_name()} " + + def get_tool_name(self) -> str: + """A function that must be implemented by derivatives to + get the clang tool's name that generated the `patched` data.""" + + raise NotImplementedError("must be implemented by derivative") def get_suggestions_from_patch( self, file_obj: FileObj, summary_only: bool, review_comments: ReviewComments @@ -118,7 +149,6 @@ def get_suggestions_from_patch( Results are stored in the ``review_comments`` parameter (passed by reference). """ - assert ( self.patched ), f"{self.__class__.__name__} has no suggestions for {file_obj.name}" @@ -130,9 +160,12 @@ def get_suggestions_from_patch( context_lines=0, # exclude any surrounding unchanged lines flag=INDENT_HEURISTIC, ) - review_comments.full_patch += f"{patch.text}" + tool_name = self.get_tool_name() + 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 for hunk in patch.hunks: - review_comments.total += 1 + review_comments.tool_total[tool_name] += 1 if summary_only: continue new_hunk_range = file_obj.is_hunk_contained(hunk) @@ -158,4 +191,5 @@ def get_suggestions_from_patch( else: body += f"\n```suggestion\n{suggestion}```" comment.comment = body - review_comments.suggestions.append(comment) + if not review_comments.merge_similar_suggestion(comment): + review_comments.suggestions.append(comment) diff --git a/cpp_linter/rest_api/github_api.py b/cpp_linter/rest_api/github_api.py index 466e00e9..f2aa102a 100644 --- a/cpp_linter/rest_api/github_api.py +++ b/cpp_linter/rest_api/github_api.py @@ -403,7 +403,6 @@ def post_review( return # don't post reviews body = f"{COMMENT_MARKER}## Cpp-linter Review\n" payload_comments = [] - total_changes = 0 summary_only = environ.get( "CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "false" ).lower() in ("true", "on", "1") @@ -412,20 +411,19 @@ def post_review( advice.append("clang-format") if tidy_review: advice.append("clang-tidy") + review_comments = ReviewComments() for tool_name in advice: - review_comments = ReviewComments() self.create_review_comments( files=files, tidy_tool=tool_name == "clang-tidy", summary_only=summary_only, review_comments=review_comments, ) - (summary, comments) = review_comments.serialize_to_github_payload(tool_name) - if not summary_only: - payload_comments.extend(comments) - body += summary - total_changes += review_comments.total - if total_changes: + (summary, comments) = review_comments.serialize_to_github_payload() + if not summary_only: + payload_comments.extend(comments) + body += summary + if sum(review_comments.tool_total.values()): event = "REQUEST_CHANGES" else: if no_lgtm: diff --git a/tests/reviews/test_pr_review.py b/tests/reviews/test_pr_review.py index d328b83e..3f7de7aa 100644 --- a/tests/reviews/test_pr_review.py +++ b/tests/reviews/test_pr_review.py @@ -53,6 +53,7 @@ def mk_param_set(**kwargs) -> OrderedDict: tuple(mk_param_set(force_approved=True).values()), tuple(mk_param_set(force_approved=True, no_lgtm=True).values()), tuple(mk_param_set(tidy_review=True, format_review=False).values()), + tuple(mk_param_set(tidy_review=True, format_review=True).values()), tuple(mk_param_set(format_review=True).values()), tuple(mk_param_set(tidy_review=True, changes=1).values()), tuple(mk_param_set(tidy_review=True, changes=0).values()), @@ -66,6 +67,7 @@ def mk_param_set(**kwargs) -> OrderedDict: "approved", "no_lgtm", "tidy", # changes == diff_chunks only + "tidy+format", # changes == diff_chunks only "format", # changes == diff_chunks only "lines_added", "all_lines", From 3a0d6b6904a413724f2cc3b136c26c048c8a70d6 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 5 Sep 2024 14:41:41 -0700 Subject: [PATCH 07/12] fix clang-tidy count of suggestions --- cpp_linter/clang_tools/clang_tidy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp_linter/clang_tools/clang_tidy.py b/cpp_linter/clang_tools/clang_tidy.py index 51525572..af4821ba 100644 --- a/cpp_linter/clang_tools/clang_tidy.py +++ b/cpp_linter/clang_tools/clang_tidy.py @@ -156,8 +156,8 @@ def _has_related_suggestion(suggestion: Suggestion) -> bool: body += f"{fixit_line}\n" body += "```\n" suggestion.comment = body + review_comments.tool_total["clang-tidy"] += 1 if not _has_related_suggestion(suggestion): - review_comments.tool_total["clang-tidy"] += 1 review_comments.suggestions.append(suggestion) From c08b6af4a0470024ab186fafcf8c93fa36796853 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 6 Sep 2024 12:45:10 -0700 Subject: [PATCH 08/12] switch to cspell pre-commit hook and update config --- .pre-commit-config.yaml | 8 +++----- cspell.config.yml | 29 +++++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0fe15440..2be776bc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -32,9 +32,7 @@ repos: - pytest - requests-mock - '.' - - repo: https://github.com/codespell-project/codespell - rev: v2.2.6 + - repo: https://github.com/streetsidesoftware/cspell-cli + rev: v8.13.3 hooks: - - id: codespell - additional_dependencies: - - tomli + - id: cspell diff --git a/cspell.config.yml b/cspell.config.yml index db8552e6..563d93c8 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -4,8 +4,11 @@ words: - argnames - argvalues - automodule + - bndy - bugprone + - bysource - caplog + - capsys - codecov - codespell - consts @@ -14,30 +17,48 @@ words: - docutils - endgroup - Fixit + - fontawesome - gitmodules + - gmtime + - intersphinx - iomanip - keepends + - levelno - libgit + - libvips - markdownlint - maxsplit + - mktime + - mypy - posix - pybind - pygit + - pypi + - pyproject + - pytest - ratelimit - revparse + - seealso - setenv + - shenxianpeng + - srcdir + - stddef - tada - toctree - tofile - tomli - - unidiff + - undoc - vararg - venv + - viewcode ignorePaths: - .env/** - .venv/** - env/** - venv/** - - tests/**/*.json - - '**.clang-tidy' - - '**.clang-format' + - tests/**/*.{json,h,c,cpp,hpp,patch,diff} + - "**.clang-tidy" + - "**.clang-format" + - pyproject.toml + - .gitignore + - "**/*.{yml,yaml,txt}" From eba64300fecd4032e348f8be42b893ec99d1ee07 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 6 Sep 2024 12:59:24 -0700 Subject: [PATCH 09/12] fix ruff lint --- tests/test_rate_limits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_rate_limits.py b/tests/test_rate_limits.py index 7c08288f..9d24009c 100644 --- a/tests/test_rate_limits.py +++ b/tests/test_rate_limits.py @@ -41,5 +41,5 @@ def test_rate_limit(monkeypatch: pytest.MonkeyPatch, response_headers: Dict[str, # ensure function exits early with pytest.raises(SystemExit) as exc: gh_client.api_request(url) - assert exc.type == SystemExit + assert exc.type is SystemExit assert exc.value.code == 1 From 2a50ed7c64b0607b0ec8e273c1aa86f7336ada45 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 6 Sep 2024 13:00:07 -0700 Subject: [PATCH 10/12] bump setuptools version per @shenxianpeng advice --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 8a4c1622..bf5348b6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,5 @@ [build-system] -requires = ["setuptools>=61", "setuptools-scm"] +requires = ["setuptools>=62.6", "setuptools-scm"] build-backend = "setuptools.build_meta" [project] From b896f887ceb0ad38ef6b012945be424b86055cf1 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 6 Sep 2024 23:50:53 -0700 Subject: [PATCH 11/12] self review --- cpp_linter/clang_tools/patcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp_linter/clang_tools/patcher.py b/cpp_linter/clang_tools/patcher.py index c92efeb2..7354f773 100644 --- a/cpp_linter/clang_tools/patcher.py +++ b/cpp_linter/clang_tools/patcher.py @@ -183,7 +183,7 @@ def get_suggestions_from_patch( if line.origin in ("+", " "): suggestion += f"{line.content}" else: - line_numb = line.new_lineno + line_numb = line.old_lineno removed.append(line_numb) if not suggestion and removed: body += "\nPlease remove the line(s)\n- " From 3bb8f33f29fc67abc55de6bb6f8c7d421c0612d0 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 6 Sep 2024 23:59:39 -0700 Subject: [PATCH 12/12] update pre-commit hooks --- .pre-commit-config.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2be776bc..030c5ae7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.5.0 + rev: v4.6.0 hooks: - id: trailing-whitespace exclude: ^tests/.*\.(?:patch|diff)$ @@ -15,14 +15,14 @@ repos: args: ["--fix=lf"] - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.3.3 + rev: v0.6.4 hooks: # Run the linter. - id: ruff # Run the formatter. - id: ruff-format - repo: https://github.com/pre-commit/mirrors-mypy - rev: 'v1.9.0' + rev: 'v1.11.2' hooks: - id: mypy additional_dependencies: