Skip to content

Commit a44161d

Browse files
2bndy52bndy5
authored
PR Review Suggestions (#51)
* get diff of suggestions for each files (from each clang tool) * support PR reviews for individual tool's output - keep suggestions within 1 diff hunk - use default SHA for review commit_id - only create a Patch when creating a review - don't post reviews for PRs marked as "draft" or "closed" * add test * allow 2% drop (maximum) in coverage on CI checks * get file changes for review when analyzing all files
1 parent 3cca935 commit a44161d

24 files changed

+1524
-65
lines changed

‎.pre-commit-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ repos:
33
rev: v4.5.0
44
hooks:
55
- id: trailing-whitespace
6-
exclude: tests/capture_tools_output/cpp-linter/cpp-linter/test_git_lib.patch
6+
exclude: ^tests/.*\.(?:patch|diff)$
77
- id: end-of-file-fixer
88
- id: check-docstring-first
99
- id: check-added-large-files

‎codecov.yml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
coverage:
2+
status:
3+
patch:
4+
default:
5+
informational: true
6+
project:
7+
default:
8+
target: auto
9+
# adjust accordingly based on how flaky your tests are
10+
# this allows a 2% drop from the previous base commit coverage
11+
threshold: 2%

‎cpp_linter/__init__.py‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ def main():
2323

2424
rest_api_client = GithubApiClient()
2525
logger.info("processing %s event", rest_api_client.event_name)
26+
is_pr_event = rest_api_client.event_name == "pull_request"
2627

2728
# set logging verbosity
2829
logger.setLevel(10 if args.verbosity or rest_api_client.debug_enabled else 20)
@@ -46,10 +47,27 @@ def main():
4647
not_ignored,
4748
args.lines_changed_only,
4849
)
49-
if files:
50-
rest_api_client.verify_files_are_present(files)
50+
rest_api_client.verify_files_are_present(files)
5151
else:
5252
files = list_source_files(args.extensions, ignored, not_ignored)
53+
# at this point, files have no info about git changes.
54+
# for PR reviews, we need this info
55+
if is_pr_event and (args.tidy_review or args.format_review):
56+
# get file changes from diff
57+
git_changes = rest_api_client.get_list_of_changed_files(
58+
args.extensions,
59+
ignored,
60+
not_ignored,
61+
lines_changed_only=0, # prevent filtering out unchanged files
62+
)
63+
# merge info from git changes into list of all files
64+
for git_file in git_changes:
65+
for file in files:
66+
if git_file.name == file.name:
67+
file.additions = git_file.additions
68+
file.diff_chunks = git_file.diff_chunks
69+
file.lines_added = git_file.lines_added
70+
break
5371
if not files:
5472
logger.info("No source files need checking!")
5573
else:
@@ -67,6 +85,8 @@ def main():
6785
args.lines_changed_only,
6886
args.database,
6987
args.extra_arg,
88+
is_pr_event and args.tidy_review,
89+
is_pr_event and args.format_review,
7090
)
7191

7292
start_log_group("Posting comment(s)")
@@ -79,6 +99,8 @@ def main():
7999
args.step_summary,
80100
args.file_annotations,
81101
args.style,
102+
args.format_review,
103+
args.tidy_review,
82104
)
83105
end_log_group()
84106

‎cpp_linter/clang_tools/__init__.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ def capture_clang_tools_output(
3939
lines_changed_only: int,
4040
database: str,
4141
extra_args: List[str],
42+
tidy_review: bool,
43+
format_review: bool,
4244
) -> Tuple[List[FormatAdvice], List[TidyAdvice]]:
4345
"""Execute and capture all output from clang-tidy and clang-format. This aggregates
4446
results in the :attr:`~cpp_linter.Globals.OUTPUT`.
@@ -54,6 +56,10 @@ def capture_clang_tools_output(
5456
:param database: The path to the compilation database.
5557
:param extra_args: A list of extra arguments used by clang-tidy as compiler
5658
arguments.
59+
:param tidy_review: A flag to enable/disable creating a diff suggestion for
60+
PR review comments using clang-tidy.
61+
:param format_review: A flag to enable/disable creating a diff suggestion for
62+
PR review comments using clang-format.
5763
"""
5864

5965
def show_tool_version_output(cmd: str): # show version output for executable used
@@ -81,7 +87,7 @@ def show_tool_version_output(cmd: str): # show version output for executable us
8187
db_json = json.loads(db_path.read_text(encoding="utf-8"))
8288

8389
# temporary cache of parsed notifications for use in log commands
84-
tidy_notes: List[TidyAdvice] = []
90+
tidy_notes = []
8591
format_advice = []
8692
for file in files:
8793
start_log_group(f"Performing checkup on {file.name}")
@@ -95,11 +101,14 @@ def show_tool_version_output(cmd: str): # show version output for executable us
95101
database,
96102
extra_args,
97103
db_json,
104+
tidy_review,
98105
)
99106
)
100107
if format_cmd is not None:
101108
format_advice.append(
102-
run_clang_format(format_cmd, file, style, lines_changed_only)
109+
run_clang_format(
110+
format_cmd, file, style, lines_changed_only, format_review
111+
)
103112
)
104113
end_log_group()
105114
return (format_advice, tidy_notes)

‎cpp_linter/clang_tools/clang_format.py‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
"""Parse output from clang-format's XML suggestions."""
22
from pathlib import PurePath
33
import subprocess
4-
from typing import List, cast
4+
from typing import List, cast, Optional
5+
56
import xml.etree.ElementTree as ET
7+
68
from ..common_fs import get_line_cnt_from_cols, FileObj
79
from ..loggers import logger
810

@@ -66,6 +68,9 @@ def __init__(self, filename: str):
6668
"""A list of `FormatReplacementLine` representing replacement(s)
6769
on a single line."""
6870

71+
#: A buffer of the applied fixes from clang-format
72+
self.patched: Optional[bytes] = None
73+
6974
def __repr__(self) -> str:
7075
return (
7176
f"<XMLFixit with {len(self.replaced_lines)} lines of "
@@ -140,6 +145,7 @@ def run_clang_format(
140145
file_obj: FileObj,
141146
style: str,
142147
lines_changed_only: int,
148+
format_review: bool,
143149
) -> FormatAdvice:
144150
"""Run clang-format on a certain file
145151
@@ -149,6 +155,8 @@ def run_clang_format(
149155
use the relative-most .clang-format configuration file.
150156
:param lines_changed_only: A flag that forces focus on only changes in the event's
151157
diff info.
158+
:param format_review: A flag to enable/disable creating a diff suggestion for
159+
PR review comments.
152160
"""
153161
cmds = [
154162
command,
@@ -168,6 +176,13 @@ def run_clang_format(
168176
logger.debug(
169177
"%s raised the following error(s):\n%s", cmds[0], results.stderr.decode()
170178
)
171-
return parse_format_replacements_xml(
179+
advice = parse_format_replacements_xml(
172180
results.stdout.decode(encoding="utf-8").strip(), file_obj, lines_changed_only
173181
)
182+
if format_review:
183+
del cmds[2] # remove `--output-replacements-xml` flag
184+
# get formatted file from stdout
185+
formatted_output = subprocess.run(cmds, capture_output=True, check=True)
186+
# store formatted_output (for comparing later)
187+
advice.patched = formatted_output.stdout
188+
return advice

‎cpp_linter/clang_tools/clang_tidy.py‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
import re
66
import subprocess
77
from typing import Tuple, Union, List, cast, Optional, Dict
8-
from pygit2 import Patch # type: ignore[import]
98
from ..loggers import logger
109
from ..common_fs import FileObj
1110

@@ -91,10 +90,19 @@ def __repr__(self) -> str:
9190

9291
class TidyAdvice:
9392
def __init__(self, notes: List[TidyNotification]) -> None:
94-
#: A patch of the suggested fixes from clang-tidy
95-
self.suggestion: Optional[Patch] = None
93+
#: A buffer of the applied fixes from clang-tidy
94+
self.patched: Optional[bytes] = None
9695
self.notes = notes
9796

97+
def diagnostics_in_range(self, start: int, end: int) -> str:
98+
"""Get a markdown formatted list of diagnostics found between a ``start``
99+
and ``end`` range of lines."""
100+
diagnostics = ""
101+
for note in self.notes:
102+
if note.line in range(start, end + 1): # range is inclusive
103+
diagnostics += f"- {note.rationale} [{note.diagnostic_link}]\n"
104+
return diagnostics
105+
98106

99107
def run_clang_tidy(
100108
command: str,
@@ -104,6 +112,7 @@ def run_clang_tidy(
104112
database: str,
105113
extra_args: List[str],
106114
db_json: Optional[List[Dict[str, str]]],
115+
tidy_review: bool,
107116
) -> TidyAdvice:
108117
"""Run clang-tidy on a certain file.
109118
@@ -134,6 +143,8 @@ def run_clang_tidy(
134143
:param db_json: The compilation database deserialized from JSON, only if
135144
``database`` parameter points to a valid path containing a
136145
``compile_commands.json file``.
146+
:param tidy_review: A flag to enable/disable creating a diff suggestion for
147+
PR review comments.
137148
"""
138149
filename = file_obj.name.replace("/", os.sep)
139150
cmds = [command]
@@ -162,7 +173,21 @@ def run_clang_tidy(
162173
logger.debug(
163174
"clang-tidy made the following summary:\n%s", results.stderr.decode()
164175
)
165-
return parse_tidy_output(results.stdout.decode(), database=db_json)
176+
177+
advice = parse_tidy_output(results.stdout.decode(), database=db_json)
178+
179+
if tidy_review:
180+
# clang-tidy overwrites the file contents when applying fixes.
181+
# create a cache of original contents
182+
original_buf = Path(file_obj.name).read_bytes()
183+
cmds.insert(1, "--fix-errors") # include compiler-suggested fixes
184+
# run clang-tidy again to apply any fixes
185+
subprocess.run(cmds, check=True)
186+
# store the modified output from clang-tidy
187+
advice.patched = Path(file_obj.name).read_bytes()
188+
# re-write original file contents (can probably skip this on CI runners)
189+
Path(file_obj.name).write_bytes(original_buf)
190+
return advice
166191

167192

168193
def parse_tidy_output(

‎cpp_linter/cli.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,22 @@
281281
specified as positional arguments will be exempt from
282282
explicitly ignored domains (see :std:option:`--ignore`).""",
283283
)
284+
cli_arg_parser.add_argument(
285+
"--tidy-review",
286+
"-tr",
287+
default="false",
288+
type=lambda input: input.lower() == "true",
289+
help="""Set to ``true`` to enable PR review suggestions
290+
from clang-tidy.""",
291+
)
292+
cli_arg_parser.add_argument(
293+
"--format-review",
294+
"-fr",
295+
default="false",
296+
type=lambda input: input.lower() == "true",
297+
help="""Set to ``true`` to enable PR review suggestions
298+
from clang-format.""",
299+
)
284300

285301

286302
def parse_ignore_option(

‎cpp_linter/common_fs.py‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
from os import environ
22
from os.path import commonpath
33
from pathlib import PurePath, Path
4-
from typing import List, Dict, Any, Union, Tuple
4+
from typing import List, Dict, Any, Union, Tuple, Optional
5+
from pygit2 import DiffHunk # type: ignore
56
from .loggers import logger, start_log_group
67

78
#: A path to generated cache artifacts. (only used when verbosity is in debug mode)
@@ -19,16 +20,21 @@ class FileObj:
1920
for all hunks in the diff.
2021
"""
2122

22-
def __init__(self, name: str, additions: List[int], diff_chunks: List[List[int]]):
23+
def __init__(
24+
self,
25+
name: str,
26+
additions: Optional[List[int]] = None,
27+
diff_chunks: Optional[List[List[int]]] = None,
28+
):
2329
self.name: str = name #: The file name
24-
self.additions: List[int] = additions
30+
self.additions: List[int] = additions or []
2531
"""A list of line numbers that contain added changes. This will be empty if
2632
not focusing on lines changed only."""
27-
self.diff_chunks: List[List[int]] = diff_chunks
33+
self.diff_chunks: List[List[int]] = diff_chunks or []
2834
"""A list of line numbers that define the beginning and ending of hunks in the
2935
diff. This will be empty if not focusing on lines changed only."""
3036
self.lines_added: List[List[int]] = FileObj._consolidate_list_to_ranges(
31-
additions
37+
additions or []
3238
)
3339
"""A list of line numbers that define the beginning and ending of ranges that
3440
have added changes. This will be empty if not focusing on lines changed only.
@@ -93,6 +99,39 @@ def serialize(self) -> Dict[str, Any]:
9399
},
94100
}
95101

102+
def is_hunk_contained(self, hunk: DiffHunk) -> Optional[Tuple[int, int]]:
103+
"""Does a given ``hunk`` start and end within a single diff hunk?
104+
105+
This also includes some compensations for hunk headers that are oddly formed.
106+
107+
.. tip:: This is mostly useful to create comments that can be posted within a
108+
git changes' diff. Ideally, designed for PR reviews based on patches
109+
generated by clang tools' output.
110+
111+
:returns: The appropriate starting and ending line numbers of the given hunk.
112+
If hunk cannot fit in a single hunk, this returns `None`.
113+
"""
114+
if hunk.old_lines > 0:
115+
start = hunk.old_start
116+
# span of old_lines is an inclusive range
117+
end = hunk.old_start + hunk.old_lines - 1
118+
else: # if number of old lines is 0
119+
# start hunk at new line number
120+
start = hunk.new_start
121+
# make it span 1 line
122+
end = start
123+
for hunk in self.diff_chunks:
124+
chunk_range = range(hunk[0], hunk[1])
125+
if start in chunk_range and end in chunk_range:
126+
return (start, end)
127+
logger.warning(
128+
"lines %d - %d are not within a single diff hunk for file %s.",
129+
start,
130+
end,
131+
self.name,
132+
)
133+
return None
134+
96135

97136
def is_file_in_list(paths: List[str], file_name: str, prompt: str) -> bool:
98137
"""Determine if a file is specified in a list of paths and/or filenames.
@@ -195,7 +234,7 @@ def list_source_files(
195234
if is_file_in_list(
196235
not_ignored, file_path, "not ignored"
197236
) or not is_file_in_list(ignored, file_path, "ignored"):
198-
files.append(FileObj(file_path, [], []))
237+
files.append(FileObj(file_path))
199238
return files
200239

201240

‎cpp_linter/git/git_str.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
DIFF_FILE_NAME = re.compile(r"^\+\+\+\sb?/(.*)$", re.MULTILINE)
1212
DIFF_RENAMED_FILE = re.compile(r"^rename to (.*)$", re.MULTILINE)
1313
DIFF_BINARY_FILE = re.compile(r"^Binary\sfiles\s", re.MULTILINE)
14-
HUNK_INFO = re.compile(r"@@\s\-\d+,\d+\s\+(\d+,\d+)\s@@", re.MULTILINE)
14+
HUNK_INFO = re.compile(r"^@@\s\-\d+,?\d*\s\+(\d+,?\d*)\s@@", re.MULTILINE)
1515

1616

1717
def _get_filename_from_diff(front_matter: str) -> Optional[re.Match]:
@@ -94,7 +94,11 @@ def _parse_patch(full_patch: str) -> Tuple[List[List[int]], List[int]]:
9494
for index, chunk in enumerate(chunks):
9595
if index % 2 == 1:
9696
# each odd element holds the starting line number and number of lines
97-
start_line, hunk_length = [int(x) for x in chunk.split(",")]
97+
if "," in chunk:
98+
start_line, hunk_length = [int(x) for x in chunk.split(",")]
99+
else:
100+
start_line = int(chunk)
101+
hunk_length = 1
98102
ranges.append([start_line, hunk_length + start_line])
99103
line_numb_in_diff = start_line
100104
continue

‎cpp_linter/loggers.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ def log_response_msg(response_buffer: Response) -> bool:
5151
"""
5252
if response_buffer.status_code >= 400:
5353
logger.error(
54-
"response returned %d message: %s",
54+
"response returned %d from %s with message: %s",
5555
response_buffer.status_code,
56+
response_buffer.url,
5657
response_buffer.text,
5758
)
5859
return False

0 commit comments

Comments
 (0)