Skip to content

Commit baee3c2

Browse files
2bndy52bndy5
authored
fix: enhance parsing paginated diffs (#125)
- fixes libgit2 diff parsing errors in paginated responses about changed files' diff. - allows renamed files to be scanned entirely when the source file's content has not changed (when using paginated responses) - add tests, but ignores coverage for critical errors (like malformed JSON responses)
1 parent a6f4e5d commit baee3c2

File tree

6 files changed

+76
-9
lines changed

6 files changed

+76
-9
lines changed

‎cpp_linter/loggers.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212

1313
logging.basicConfig(
1414
format="%(name)s: %(message)s",
15-
handlers=[RichHandler(show_time=False)],
15+
handlers=[RichHandler(show_time=False, show_path=False)],
1616
)
1717

1818
except ImportError: # pragma: no cover

‎cpp_linter/rest_api/__init__.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ class RestApiClient(ABC):
4040
def __init__(self, rate_limit_headers: RateLimitHeaders) -> None:
4141
self.session = requests.Session()
4242

43+
#: The brand name of the git server that provides the REST API.
44+
self._name: str = "Generic"
45+
4346
# The remain API requests allowed under the given token (if any).
4447
self._rate_limit_remaining = -1 # -1 means unknown
4548
# a counter for avoiding secondary rate limits
@@ -53,7 +56,8 @@ def _rate_limit_exceeded(self):
5356
logger.error("RATE LIMIT EXCEEDED!")
5457
if self._rate_limit_reset is not None:
5558
logger.error(
56-
"Gitlab REST API rate limit resets on %s",
59+
"%s REST API rate limit resets on %s",
60+
self._name,
5761
time.strftime("%d %B %Y %H:%M +0000", self._rate_limit_reset),
5862
)
5963
sys.exit(1)

‎cpp_linter/rest_api/github_api.py‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ def __init__(self) -> None:
4646
# create default headers to be used for all HTTP requests
4747
self.session.headers.update(self.make_headers())
4848

49+
self._name = "GitHub"
50+
4951
#: The base domain for the REST API
5052
self.api_url = environ.get("GITHUB_API_URL", "https://api.github.com")
5153
#: The ``owner``/``repository`` name.
@@ -134,18 +136,37 @@ def _get_changed_files_paginated(
134136
else:
135137
file_list = response.json()["files"]
136138
for file in file_list:
137-
assert "filename" in file
138-
file_name = file["filename"]
139+
try:
140+
file_name = file["filename"]
141+
except KeyError as exc: # pragma: no cover
142+
logger.error(
143+
f"Missing 'filename' key in file:\n{json.dumps(file, indent=2)}"
144+
)
145+
raise exc
139146
if not file_filter.is_source_or_ignored(file_name):
140147
continue
148+
if lines_changed_only > 0 and cast(int, file.get("changes", 0)) == 0:
149+
continue # also prevents KeyError below when patch is not provided
141150
old_name = file_name
142151
if "previous_filename" in file:
143152
old_name = file["previous_filename"]
144-
assert "patch" in file
153+
if "patch" not in file:
154+
if lines_changed_only > 0:
155+
# diff info is needed for further operations
156+
raise KeyError( # pragma: no cover
157+
f"{file_name} has no patch info:\n{json.dumps(file, indent=2)}"
158+
)
159+
elif (
160+
cast(int, file.get("changes", 0)) == 0
161+
): # in case files-changed-only is true
162+
# file was likely renamed without source changes
163+
files.append(FileObj(file_name)) # scan entire file instead
164+
continue
145165
file_diff = (
146166
f"diff --git a/{old_name} b/{file_name}\n"
147167
+ f"--- a/{old_name}\n+++ b/{file_name}\n"
148168
+ file["patch"]
169+
+ "\n"
149170
)
150171
files.extend(parse_diff(file_diff, file_filter, lines_changed_only))
151172
return files

‎tests/list_changes/pull_request_files_pg2.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,14 @@
1010
"raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp",
1111
"contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1",
1212
"patch": "@@ -5,12 +5,10 @@\n class Dummy {\n char* useless;\n int numb;\n+ Dummy() :numb(0), useless(\"\\0\"){}\n \n public:\n- void *not_usefull(char *str){\n- useless = str;\n- return 0;\n- }\n+ void *not_useful(char *str){useless = str;}\n };\n \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };"
13+
},
14+
{
15+
"sha": "17694f6803e9efd8cdceda06ea12c266793abacb",
16+
"filename": "include/test/tree.hpp",
17+
"status": "renamed",
18+
"additions": 0,
19+
"deletions": 0,
20+
"changes": 0,
21+
"previous_filename": "include/test-tree.hpp"
1322
}
1423
]

‎tests/list_changes/push_files_pg2.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@
1111
"raw_url": "https://github.com/cpp-linter/test-cpp-linter-action/raw/635a9c57bdcca07b99ddef52c2640337c50280b1/src%2Fdemo.hpp",
1212
"contents_url": "https://api.github.com/repos/cpp-linter/test-cpp-linter-action/contents/src%2Fdemo.hpp?ref=635a9c57bdcca07b99ddef52c2640337c50280b1",
1313
"patch": "@@ -5,12 +5,10 @@\n class Dummy {\n char* useless;\n int numb;\n+ Dummy() :numb(0), useless(\"\\0\"){}\n \n public:\n- void *not_usefull(char *str){\n- useless = str;\n- return 0;\n- }\n+ void *not_useful(char *str){useless = str;}\n };\n \n \n@@ -28,14 +26,11 @@ class Dummy {\n \n \n \n-\n-\n-\n-\n \n \n struct LongDiff\n {\n+\n long diff;\n \n };"
14+
},
15+
{
16+
"sha": "17694f6803e9efd8cdceda06ea12c266793abacb",
17+
"filename": "include/test/tree.hpp",
18+
"status": "renamed",
19+
"additions": 0,
20+
"deletions": 0,
21+
"changes": 0,
22+
"previous_filename": "include/test-tree.hpp"
1423
}
1524
]
1625
}

‎tests/list_changes/test_get_file_changes.py‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,40 +16,60 @@
1616

1717

1818
@pytest.mark.parametrize(
19-
"event_name,paginated,fake_runner",
19+
"event_name,paginated,fake_runner,lines_changed_only",
2020
[
2121
# push event (full diff)
2222
(
2323
"unknown", # let coverage include logged warning about unknown event
2424
False,
2525
True,
26+
1,
2627
),
2728
# pull request event (full diff)
2829
(
2930
"pull_request",
3031
False,
3132
True,
33+
1,
3234
),
3335
# push event (paginated diff)
3436
(
3537
"push", # let coverage include logged warning about unknown event
3638
True,
3739
True,
40+
1,
3841
),
3942
# pull request event (paginated diff)
4043
(
4144
"pull_request",
4245
True,
4346
True,
47+
1,
48+
),
49+
# push event (paginated diff with all lines)
50+
(
51+
"push", # let coverage include logged warning about unknown event
52+
True,
53+
True,
54+
0,
55+
),
56+
# pull request event (paginated diff with all lines)
57+
(
58+
"pull_request",
59+
True,
60+
True,
61+
0,
4462
),
4563
# local dev env
46-
("", False, False),
64+
("", False, False, 1),
4765
],
4866
ids=[
4967
"push",
5068
"pull_request",
5169
"push(paginated)",
5270
"pull_request(paginated)",
71+
"push(all-lines,paginated)",
72+
"pull_request(all-lines,paginated)",
5373
"local_dev",
5474
],
5575
)
@@ -60,6 +80,7 @@ def test_get_changed_files(
6080
event_name: str,
6181
paginated: bool,
6282
fake_runner: bool,
83+
lines_changed_only: int,
6384
):
6485
"""test getting a list of changed files for an event."""
6586
caplog.set_level(logging.DEBUG, logger=logger.name)
@@ -121,8 +142,11 @@ def test_get_changed_files(
121142
)
122143

123144
files = gh_client.get_list_of_changed_files(
124-
FileFilter(extensions=["cpp", "hpp"]), lines_changed_only=1
145+
FileFilter(extensions=["cpp", "hpp"]), lines_changed_only=lines_changed_only
125146
)
126147
assert files
127148
for file in files:
128-
assert file.name in ("src/demo.cpp", "src/demo.hpp")
149+
expected = ["src/demo.cpp", "src/demo.hpp"]
150+
if lines_changed_only == 0:
151+
expected.append("include/test/tree.hpp")
152+
assert file.name in expected

0 commit comments

Comments
 (0)