Skip to content

Commit 266c1b4

Browse files
2bndy52bndy5
committed
fix regression about filtering files changed
This reintroduces the approach where we filter the list of changed files according to diff information about lines-changed-only value. The advantage (which still holds true) is that we don't run a clang tool on a file if there is no changes of concern.
1 parent f10bfc2 commit 266c1b4

File tree

12 files changed

+83
-73
lines changed

12 files changed

+83
-73
lines changed

‎cpp_linter/__init__.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ def main():
4444
args.extensions,
4545
ignored,
4646
not_ignored,
47+
args.lines_changed_only,
4748
)
4849
if files:
4950
rest_api_client.verify_files_are_present(files)

‎cpp_linter/clang_tools/clang_format.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ def __repr__(self) -> str:
7676
def formalize_style_name(style: str) -> str:
7777
if style.startswith("llvm") or style.startswith("gnu"):
7878
return style.upper()
79-
if style not in (
79+
if style in (
8080
"google",
8181
"chromium",
8282
"microsoft",

‎cpp_linter/common_fs.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,27 @@ def is_file_in_list(paths: List[str], file_name: str, prompt: str) -> bool:
121121
return False
122122

123123

124+
def has_line_changes(
125+
lines_changed_only: int, diff_chunks: List[List[int]], additions: List[int]
126+
) -> bool:
127+
"""Does this file actually apply to condition specified by ``lines_changed_only``?
128+
129+
:param lines_changed_only: A value that means:
130+
131+
- 0 = We don't care. Analyze the whole file.
132+
- 1 = Only analyze lines in the diff chunks, which may include unchanged
133+
lines but not lines with subtractions.
134+
- 2 = Only analyze lines with additions.
135+
:param diff_chunks: The ranges of lines in the diff for a single file.
136+
:param additions: The lines with additions in the diff for a single file.
137+
"""
138+
return (
139+
(lines_changed_only == 1 and len(diff_chunks) > 0)
140+
or (lines_changed_only == 2 and len(additions) > 0)
141+
or not lines_changed_only
142+
)
143+
144+
124145
def is_source_or_ignored(
125146
file_name: str,
126147
ext_list: List[str],

‎cpp_linter/git/__init__.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
GitError,
2020
)
2121
from .. import CACHE_PATH
22-
from ..common_fs import FileObj, is_source_or_ignored
22+
from ..common_fs import FileObj, is_source_or_ignored, has_line_changes
2323
from ..loggers import logger
2424
from .git_str import parse_diff as legacy_parse_diff
2525

@@ -87,13 +87,15 @@ def parse_diff(
8787
extensions: List[str],
8888
ignored: List[str],
8989
not_ignored: List[str],
90+
lines_changed_only: int,
9091
) -> List[FileObj]:
9192
"""Parse a given diff into file objects.
9293
9394
:param diff_obj: The complete git diff object for an event.
9495
:param extensions: A list of file extensions to focus on only.
9596
:param ignored: A list of paths or files to ignore.
9697
:param not_ignored: A list of paths or files to explicitly not ignore.
98+
:param lines_changed_only: A value that dictates what file changes to focus on.
9799
:returns: A `list` of `dict` containing information about the files changed.
98100
99101
.. note:: Deleted files are omitted because we only want to analyze updates.
@@ -104,7 +106,9 @@ def parse_diff(
104106
diff_obj = Diff.parse_diff(diff_obj)
105107
except GitError as exc:
106108
logger.warning(f"pygit2.Diff.parse_diff() threw {exc}")
107-
return legacy_parse_diff(diff_obj, extensions, ignored, not_ignored)
109+
return legacy_parse_diff(
110+
diff_obj, extensions, ignored, not_ignored, lines_changed_only
111+
)
108112
for patch in diff_obj:
109113
if patch.delta.status not in ADDITIVE_STATUS:
110114
continue
@@ -113,7 +117,10 @@ def parse_diff(
113117
):
114118
continue
115119
diff_chunks, additions = parse_patch(patch.hunks)
116-
file_objects.append(FileObj(patch.delta.new_file.path, additions, diff_chunks))
120+
if has_line_changes(lines_changed_only, diff_chunks, additions):
121+
file_objects.append(
122+
FileObj(patch.delta.new_file.path, additions, diff_chunks)
123+
)
117124
return file_objects
118125

119126

‎cpp_linter/git/git_str.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
:py:meth:`pygit2.Diff.parse_diff()` function fails in `cpp_linter.git.parse_diff()`"""
44
import re
55
from typing import Optional, List, Tuple, cast
6-
from ..common_fs import FileObj, is_source_or_ignored
6+
from ..common_fs import FileObj, is_source_or_ignored, has_line_changes
77
from ..loggers import logger
88

99

@@ -36,14 +36,19 @@ def _get_filename_from_diff(front_matter: str) -> Optional[re.Match]:
3636

3737

3838
def parse_diff(
39-
full_diff: str, extensions: List[str], ignored: List[str], not_ignored: List[str]
39+
full_diff: str,
40+
extensions: List[str],
41+
ignored: List[str],
42+
not_ignored: List[str],
43+
lines_changed_only: int,
4044
) -> List[FileObj]:
4145
"""Parse a given diff into file objects.
4246
4347
:param full_diff: The complete diff for an event.
4448
:param extensions: A list of file extensions to focus on only.
4549
:param ignored: A list of paths or files to ignore.
4650
:param not_ignored: A list of paths or files to explicitly not ignore.
51+
:param lines_changed_only: A value that dictates what file changes to focus on.
4752
:returns: A `list` of `FileObj` instances containing information about the files
4853
changed.
4954
"""
@@ -65,7 +70,8 @@ def parse_diff(
6570
if not is_source_or_ignored(filename, extensions, ignored, not_ignored):
6671
continue
6772
diff_chunks, additions = _parse_patch(diff[first_hunk.start() :])
68-
file_objects.append(FileObj(filename, additions, diff_chunks))
73+
if has_line_changes(lines_changed_only, diff_chunks, additions):
74+
file_objects.append(FileObj(filename, additions, diff_chunks))
6975
return file_objects
7076

7177

‎cpp_linter/rest_api/__init__.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,18 @@ def make_headers(self, use_diff: bool = False) -> Dict[str, str]:
4444
raise NotImplementedError("must be implemented in the derivative")
4545

4646
def get_list_of_changed_files(
47-
self, extensions: List[str], ignored: List[str], not_ignored: List[str]
47+
self,
48+
extensions: List[str],
49+
ignored: List[str],
50+
not_ignored: List[str],
51+
lines_changed_only: int,
4852
) -> List[FileObj]:
4953
"""Fetch a list of the event's changed files.
5054
5155
:param extensions: A list of file extensions to focus on only.
5256
:param ignored: A list of paths or files to ignore.
5357
:param not_ignored: A list of paths or files to explicitly not ignore.
58+
:param lines_changed_only: A value that dictates what file changes to focus on.
5459
"""
5560
raise NotImplementedError("must be implemented in the derivative")
5661

‎cpp_linter/rest_api/github_api.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,11 @@ def set_exit_code(
6868
)
6969

7070
def get_list_of_changed_files(
71-
self, extensions: List[str], ignored: List[str], not_ignored: List[str]
71+
self,
72+
extensions: List[str],
73+
ignored: List[str],
74+
not_ignored: List[str],
75+
lines_changed_only: int,
7276
) -> List[FileObj]:
7377
start_log_group("Get list of specified source files")
7478
if environ.get("CI", "false") == "true":
@@ -88,9 +92,17 @@ def get_list_of_changed_files(
8892
files_link, headers=self.make_headers(use_diff=True)
8993
)
9094
log_response_msg(response_buffer)
91-
files = parse_diff(response_buffer.text, extensions, ignored, not_ignored)
95+
files = parse_diff(
96+
response_buffer.text,
97+
extensions,
98+
ignored,
99+
not_ignored,
100+
lines_changed_only,
101+
)
92102
else:
93-
files = parse_diff(get_diff(), extensions, ignored, not_ignored)
103+
files = parse_diff(
104+
get_diff(), extensions, ignored, not_ignored, lines_changed_only
105+
)
94106
return files
95107

96108
def verify_files_are_present(self, files: List[FileObj]) -> None:

‎tests/capture_tools_output/chocolate-doom/chocolate-doom/71091562db5b0e7853d08ffa2f110af49cc3bc0d/expected-result_2.json

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,4 @@
11
[
2-
{
3-
"filename": "src/hexen/info.c",
4-
"line_filter": {
5-
"diff_chunks": [
6-
[
7-
143,
8-
149
9-
],
10-
[
11-
179,
12-
185
13-
],
14-
[
15-
248,
16-
255
17-
]
18-
],
19-
"lines_added": []
20-
}
21-
},
22-
{
23-
"filename": "src/hexen/p_enemy.c",
24-
"line_filter": {
25-
"diff_chunks": [
26-
[
27-
1102,
28-
1108
29-
],
30-
[
31-
3853,
32-
3859
33-
],
34-
[
35-
3929,
36-
3935
37-
]
38-
],
39-
"lines_added": []
40-
}
41-
},
422
{
433
"filename": "src/hexen/p_local.h",
444
"line_filter": {

‎tests/capture_tools_output/libvips/libvips/fe82be345a5b654a76835a7aea5a804bd9ebff0a/expected-result_2.json

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19369,18 +19369,6 @@
1936919369
]
1937019370
}
1937119371
},
19372-
{
19373-
"filename": "libvips/colour/profiles.h",
19374-
"line_filter": {
19375-
"diff_chunks": [
19376-
[
19377-
10,
19378-
13
19379-
]
19380-
],
19381-
"lines_added": []
19382-
}
19383-
},
1938419372
{
1938519373
"filename": "libvips/colour/rad2float.c",
1938619374
"line_filter": {

‎tests/capture_tools_output/test_tools_output.py

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ def prep_tmp_dir(
105105
monkeypatch: pytest.MonkeyPatch,
106106
repo: str,
107107
commit: str,
108+
lines_changed_only: int,
108109
copy_configs: bool = False,
109110
):
110111
"""Some extra setup for test's temp directory to ensure needed files exist."""
@@ -128,7 +129,10 @@ def prep_tmp_dir(
128129
monkeypatch.chdir(str(repo_cache))
129130
CACHE_PATH.mkdir(exist_ok=True)
130131
files = gh_client.get_list_of_changed_files(
131-
extensions=["c", "h", "hpp", "cpp"], ignored=[".github"], not_ignored=[]
132+
extensions=["c", "h", "hpp", "cpp"],
133+
ignored=[".github"],
134+
not_ignored=[],
135+
lines_changed_only=lines_changed_only,
132136
)
133137
gh_client.verify_files_are_present(files)
134138
repo_path = tmp_path / repo.split("/")[1]
@@ -181,6 +185,7 @@ def test_lines_changed_only(
181185
extensions=extensions,
182186
ignored=[".github"],
183187
not_ignored=[],
188+
lines_changed_only=lines_changed_only,
184189
)
185190
if files:
186191
expected_results_json = (
@@ -191,8 +196,7 @@ def test_lines_changed_only(
191196
)
192197
### uncomment this paragraph to update/generate the expected test's results
193198
# expected_results_json.write_text(
194-
# json.dumps([f.serialize() for f in files], indent=2)
195-
# + "\n",
199+
# json.dumps([f.serialize() for f in files], indent=2) + "\n",
196200
# encoding="utf-8",
197201
# )
198202
test_result = json.loads(expected_results_json.read_text(encoding="utf-8"))
@@ -237,6 +241,7 @@ def test_format_annotations(
237241
tmp_path,
238242
monkeypatch,
239243
**TEST_REPO_COMMIT_PAIRS[0],
244+
lines_changed_only=lines_changed_only,
240245
copy_configs=True,
241246
)
242247
format_advice, tidy_advice = capture_clang_tools_output(
@@ -312,6 +317,7 @@ def test_tidy_annotations(
312317
tmp_path,
313318
monkeypatch,
314319
**TEST_REPO_COMMIT_PAIRS[4],
320+
lines_changed_only=lines_changed_only,
315321
copy_configs=False,
316322
)
317323
format_advice, tidy_advice = capture_clang_tools_output(
@@ -429,7 +435,11 @@ def test_parse_diff(
429435

430436
Path(CACHE_PATH).mkdir()
431437
files = parse_diff(
432-
get_diff(), extensions=["cpp", "hpp"], ignored=[], not_ignored=[]
438+
get_diff(),
439+
extensions=["cpp", "hpp"],
440+
ignored=[],
441+
not_ignored=[],
442+
lines_changed_only=0,
433443
)
434444
if sha == TEST_REPO_COMMIT_PAIRS[4]["commit"] or patch:
435445
assert files

‎tests/test_git_str.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,16 +40,16 @@ def test_pygit2_bug1260(caplog: pytest.LogCaptureFixture):
4040
caplog.set_level(logging.WARNING, logger=logger.name)
4141
# the bug in libgit2 should trigger a call to
4242
# cpp_linter.git_str.legacy_parse_diff()
43-
files = parse_diff(diff_str, ["cpp"], [], [])
43+
files = parse_diff(diff_str, ["cpp"], [], [], 0)
4444
assert caplog.messages, "this test is no longer needed; bug was fixed in pygit2"
4545
# if we get here test, then is satisfied
4646
assert not files # no line changes means no file to focus on
4747

4848

4949
def test_typical_diff():
5050
"""For coverage completeness. Also tests for files with spaces in the names."""
51-
from_c = parse_diff(TYPICAL_DIFF, ["cpp"], [], [])
52-
from_py = parse_diff_str(TYPICAL_DIFF, ["cpp"], [], [])
51+
from_c = parse_diff(TYPICAL_DIFF, ["cpp"], [], [], 0)
52+
from_py = parse_diff_str(TYPICAL_DIFF, ["cpp"], [], [], 0)
5353
assert [f.serialize() for f in from_c] == [f.serialize() for f in from_py]
5454
for file_obj in from_c:
5555
# file name should have spaces
@@ -65,13 +65,13 @@ def test_binary_diff():
6565
"Binary files /dev/null and b/some picture.png differ",
6666
]
6767
)
68-
files = parse_diff_str(diff_str, ["cpp"], [], [])
68+
files = parse_diff_str(diff_str, ["cpp"], [], [], 0)
6969
# binary files are ignored during parsing
7070
assert not files
7171

7272

7373
def test_ignored_diff():
7474
"""For coverage completeness"""
75-
files = parse_diff_str(TYPICAL_DIFF, ["hpp"], [], [])
75+
files = parse_diff_str(TYPICAL_DIFF, ["hpp"], [], [], 0)
7676
# binary files are ignored during parsing
7777
assert not files

‎tests/test_misc.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ def test_get_changed_files(
158158
text="",
159159
)
160160

161-
files = gh_client.get_list_of_changed_files([], [], [])
161+
files = gh_client.get_list_of_changed_files([], [], [], 0)
162162
assert not files
163163

164164

0 commit comments

Comments
 (0)