Skip to content

Hide or reuse PR reviews #135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
e9d7252
Add reuse/deletion of existing review comments
maoliver-amd Dec 2, 2024
c96c375
Add options for controlling review comment reuse/deletion.
maoliver-amd Dec 2, 2024
9f15407
Update review comment CLI options.
maoliver-amd Dec 5, 2024
63bad77
Remove unused imports.
maoliver-amd Dec 5, 2024
dd8eb38
Apply suggestions from code review
maoliver-amd Dec 6, 2024
c6e2d19
Use get instead of inline conditionals.
maoliver-amd Dec 6, 2024
d991263
Reformat code.
maoliver-amd Dec 6, 2024
263fa86
Make graphQL queries constants.
maoliver-amd Dec 6, 2024
8d63b68
Use self.api for graphql
maoliver-amd Dec 6, 2024
a7b0f34
Set delete_comment default to False.
maoliver-amd Dec 6, 2024
8239d32
Rename variables to match CLI option names.
maoliver-amd Dec 6, 2024
c6348f5
Simplify retrieving line_start/line_end from graphql response.
maoliver-amd Dec 6, 2024
2b73f2c
Fix line_start and line_end
maoliver-amd Dec 6, 2024
62f62fe
Improve review thread comment log output.
maoliver-amd Dec 6, 2024
687772e
Add graphql mock responses.
maoliver-amd Dec 6, 2024
b1122f6
Fix id tag naming.
maoliver-amd Dec 6, 2024
e7f13f7
Apply suggestions from code review
maoliver-amd Dec 7, 2024
20e3d65
Apply suggestions from code review
maoliver-amd Dec 9, 2024
e9abe5c
Formatting cleanups.
maoliver-amd Dec 9, 2024
e978e70
Improve qraphQL query error detection.
maoliver-amd Dec 9, 2024
b0a9bd6
Remove unused graphql author field
maoliver-amd Dec 10, 2024
dfb80f3
code review
2bndy5 Dec 11, 2024
70294ed
Dont dismiss ignored reviews.
maoliver-amd Dec 13, 2024
20a541e
Correct output concern count
maoliver-amd Dec 17, 2024
ea6c5d1
Add default argument to _dismiss_stale_reviews.
maoliver-amd Dec 17, 2024
3ca65bf
Extract _check_reused_comments into standalone function.
maoliver-amd Dec 17, 2024
b244350
Fix incorrect comment check.
maoliver-amd Dec 17, 2024
d0a9e47
Update _close_review_comment log.
maoliver-amd Dec 17, 2024
83ff91e
Fix _dismiss_stale_reviews default argument to None.
maoliver-amd Dec 17, 2024
c56813c
Apply suggestions from code review
maoliver-amd Dec 18, 2024
dc3ea16
Add graphql return check.
maoliver-amd Dec 18, 2024
988eb75
Remove CLI option for reuse_review_comments.
maoliver-amd Dec 18, 2024
a577087
Remove reuse_review_comments functionality.
maoliver-amd Dec 18, 2024
81eba69
Change effects to affects.
maoliver-amd Dec 18, 2024
aff2670
fix test
2bndy5 Dec 11, 2024
e5f3482
use paginated graphQL query to get review threads
2bndy5 Dec 18, 2024
822a845
deeper review
2bndy5 Dec 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 63 additions & 9 deletions cpp_linter/clang_tools/patcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,27 @@
return result


class ExistingSuggestion(Suggestion):
def __init__(self, file_name) -> None:
super().__init__(file_name)

Check warning on line 47 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L47

Added line #L47 was not covered by tests
#: The review ID corresponding to this review comment.
self.review_id: str = ""
self.review_is_minimized: bool = False
"""Is the review minimized?

Check warning on line 51 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L49-L51

Added lines #L49 - L51 were not covered by tests
This applies to entire review, not the thread or comments within."""


class ExistingThread:
def __init__(self) -> None:
self.comments: List[ExistingSuggestion] = []

Check warning on line 57 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L57

Added line #L57 was not covered by tests
#: Is the review thread resolved?
self.is_resolved: bool = False

Check warning on line 59 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L59

Added line #L59 was not covered by tests
#: Is the review thread collapsed?
self.is_collapsed: bool = False

Check warning on line 61 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L61

Added line #L61 was not covered by tests
#: The review thread's ``node_id``.
self.id: str = ""

Check warning on line 63 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L63

Added line #L63 was not covered by tests


class ReviewComments:
"""A data structure to contain PR review comments from a specific clang tool."""

Expand All @@ -66,6 +87,12 @@
"""The full patch of all the suggestions (including those that will not
fit within the diff)"""

self.tool_reused: Dict[str, int] = {
"clang-tidy": 0,
"clang-format": 0,
}
"""The total number of reused concerns from previous reviews."""

def merge_similar_suggestion(self, suggestion: Suggestion) -> bool:
"""Merge a given ``suggestion`` into a similar `Suggestion`

Expand Down Expand Up @@ -105,10 +132,10 @@
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
posted_tool_advice["clang-tidy"] += comment.comment.count("### clang-tidy")
posted_tool_advice["clang-format"] += comment.comment.count(
"### clang-format"
)

for tool_name in ("clang-tidy", "clang-format"):
tool_version = tidy_version
Expand All @@ -117,15 +144,18 @@
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)
and posted_tool_advice[tool_name] != self.tool_total[tool_name]
if len(comments) and (
posted_tool_advice[tool_name] != self.tool_total[tool_name]
or self.tool_reused[tool_name] != 0
):
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"
+ f"{self.tool_total[tool_name]} _new_ {tool_name}"
+ " concerns fit within this pull request's diff."
)
if self.tool_reused[tool_name] != 0:
summary += f" {self.tool_reused[tool_name]} concerns were suppressed as duplicates."

Check warning on line 157 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L157

Added line #L157 was not covered by tests
summary += "\n"
if self.full_patch[tool_name]:
summary += (
f"\n<details><summary>Click here for the full {tool_name} patch"
Expand All @@ -136,6 +166,30 @@
summary += f"No concerns from {tool_name}.\n"
return (summary, comments)

def remove_reused_suggestions(self, existing_review_comments: List[Suggestion]):
"""Remove any reused ``Suggestion`` from the internal list and update counts"""
if not existing_review_comments:
return
review_comments_suggestions = self.suggestions
self.suggestions = []
clang_tidy_comments = 0
clang_format_comments = 0
for suggestion in review_comments_suggestions:
if suggestion not in existing_review_comments:
clang_tidy_comments += suggestion.comment.count("### clang-tidy")
clang_format_comments += suggestion.comment.count("### clang-format")
self.suggestions.append(suggestion)
if clang_tidy_comments > 0 and self.tool_total["clang-tidy"] is not None:
self.tool_reused["clang-tidy"] = (

Check warning on line 183 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L171-L183

Added lines #L171 - L183 were not covered by tests
self.tool_total["clang-tidy"] - clang_tidy_comments
)
self.tool_total["clang-tidy"] = clang_tidy_comments
if clang_format_comments > 0 and self.tool_total["clang-format"] is not None:
self.tool_reused["clang-format"] = (

Check warning on line 188 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L186-L188

Added lines #L186 - L188 were not covered by tests
self.tool_total["clang-format"] - clang_format_comments
)
self.tool_total["clang-format"] = clang_format_comments

Check warning on line 191 in cpp_linter/clang_tools/patcher.py

View check run for this annotation

Codecov / codecov/patch

cpp_linter/clang_tools/patcher.py#L191

Added line #L191 was not covered by tests


class PatchMixin(ABC):
"""An abstract mixin that unified parsing of the suggestions into
Expand Down
12 changes: 12 additions & 0 deletions cpp_linter/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class Args(UserDict):
ignore_format: str = ""
#: See :std:option:`--passive-reviews`.
passive_reviews: bool = False
#: See :std:option:`--delete-review-comments`.
delete_review_comments: bool = False


_parser_args: Dict[Sequence[str], Any] = {}
Expand Down Expand Up @@ -358,6 +360,16 @@ class Args(UserDict):
help="""Set to ``true`` to prevent Pull Request
reviews from requesting or approving changes.""",
)
_parser_args[("-C", "--delete-review-comments")] = dict(
default="false",
type=lambda input: input.lower() == "true",
help="""Set to ``true`` to delete existing outdated/unused
Pull request review comments, ``false`` to just set them to resolved.
This only affects review comments made when either
:std:option:`--tidy-review` or :std:option:`--format-review` is enabled.

Defaults to ``%(default)s``.""",
)


def _parse_jobs(val: str) -> Optional[int]:
Expand Down
Loading
Loading