From afd3a850fa8d6b9bcaec39c2905dab421a54938b Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 1 Nov 2024 19:46:06 +0100 Subject: [PATCH 1/4] fix: resolve `PTH*` lints --- .../clang_tidy_review/__init__.py | 51 +++++++++---------- .../clang_tidy_review/review.py | 7 ++- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 2978a39..0232b57 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -7,7 +7,6 @@ import base64 import contextlib import fnmatch -import glob import io import itertools import json @@ -19,6 +18,7 @@ import textwrap import zipfile from operator import itemgetter +from pathlib import Path from typing import Dict, List, Optional, TypedDict import unidiff @@ -251,7 +251,7 @@ def config_file_or_checks( def load_clang_tidy_warnings(): """Read clang-tidy warnings from FIXES_FILE. Can be produced by build_clang_tidy_warnings""" try: - with open(FIXES_FILE) as fixes_file: + with Path(FIXES_FILE).open() as fixes_file: return yaml.safe_load(fixes_file) except FileNotFoundError: return {} @@ -402,12 +402,12 @@ def make_file_offset_lookup(filenames): lookup = {} for filename in filenames: - with open(filename) as file: + with Path(filename).open() as file: lines = file.readlines() # Length of each line line_lengths = map(len, lines) # Cumulative sum of line lengths => offset at end of each line - lookup[os.path.abspath(filename)] = [ + lookup[Path(filename).resolve().as_posix()] = [ 0, *list(itertools.accumulate(line_lengths)), ] @@ -427,22 +427,21 @@ def get_diagnostic_file_path(clang_tidy_diagnostic, build_dir): file_path = clang_tidy_diagnostic["DiagnosticMessage"]["FilePath"] if file_path == "": return "" - if os.path.isabs(file_path): - return os.path.normpath(os.path.abspath(file_path)) + file_path = Path(file_path) + if file_path.is_absolute(): + return os.path.normpath(file_path.resolve()) if "BuildDirectory" in clang_tidy_diagnostic: return os.path.normpath( - os.path.abspath( - os.path.join(clang_tidy_diagnostic["BuildDirectory"], file_path) - ) + (Path(clang_tidy_diagnostic["BuildDirectory"]) / file_path).resolve() ) - return os.path.normpath(os.path.abspath(file_path)) + return os.path.normpath(file_path.resolve()) # Pre-clang-tidy-9 format if "FilePath" in clang_tidy_diagnostic: file_path = clang_tidy_diagnostic["FilePath"] if file_path == "": return "" - return os.path.normpath(os.path.abspath(os.path.join(build_dir, file_path))) + return os.path.normpath((Path(build_dir) / file_path).resolve()) return "" @@ -469,7 +468,7 @@ def find_line_number_from_offset(offset_lookup, filename, offset): def read_one_line(filename, line_offset): """Read a single line from a source file""" # Could cache the files instead of opening them each time? - with open(filename) as file: + with Path(filename).open() as file: file.seek(line_offset) return file.readline().rstrip("\n") @@ -493,7 +492,7 @@ def collate_replacement_sets(diagnostic, offset_lookup): # from the FilePath and we'll end up looking for a path that's not in # the lookup dict # To fix this, we'll convert all the FilePaths to absolute paths - replacement["FilePath"] = os.path.abspath(replacement["FilePath"]) + replacement["FilePath"] = Path(replacement["FilePath"]).resolve().as_posix() # It's possible the replacement is needed in another file? # Not really sure how that could come about, but let's @@ -661,7 +660,7 @@ def fix_absolute_paths(build_compile_commands, base_dir): print(f"Found '{build_compile_commands}', updating absolute paths") # We might need to change some absolute paths if we're inside # a docker container - with open(build_compile_commands) as f: + with Path(build_compile_commands).open() as f: compile_commands = json.load(f) print(f"Replacing '{basedir}' with '{newbasedir}'", flush=True) @@ -670,7 +669,7 @@ def fix_absolute_paths(build_compile_commands, base_dir): str(basedir), str(newbasedir) ) - with open(build_compile_commands, "w") as f: + with Path(build_compile_commands).open("w") as f: f.write(modified_compile_commands) @@ -931,7 +930,7 @@ def create_review( if files == []: with message_group("No files to check!"): - with open(REVIEW_FILE, "w") as review_file: + with Path(REVIEW_FILE).open("w") as review_file: json.dump( { "body": "clang-tidy found no files to check", @@ -947,7 +946,7 @@ def create_review( line_ranges = get_line_ranges(diff, files) if line_ranges == "[]": with message_group("No lines added in this PR!"): - with open(REVIEW_FILE, "w") as review_file: + with Path(REVIEW_FILE).open("w") as review_file: json.dump( { "body": "clang-tidy found no lines added", @@ -997,7 +996,7 @@ def create_review( review = create_review_file( clang_tidy_warnings, diff_lookup, offset_lookup, build_dir ) - with open(REVIEW_FILE, "w") as review_file: + with Path(REVIEW_FILE).open("w") as review_file: json.dump(review, review_file) return review @@ -1049,7 +1048,7 @@ def load_metadata() -> Optional[Metadata]: print(f"WARNING: Could not find metadata file ('{METADATA_FILE}')", flush=True) return None - with open(METADATA_FILE) as metadata_file: + with Path(METADATA_FILE).open() as metadata_file: return json.load(metadata_file) @@ -1058,7 +1057,7 @@ def save_metadata(pr_number: int) -> None: metadata: Metadata = {"pr_number": pr_number} - with open(METADATA_FILE, "w") as metadata_file: + with Path(METADATA_FILE).open("w") as metadata_file: json.dump(metadata, metadata_file) @@ -1069,15 +1068,15 @@ def load_review(review_file: pathlib.Path) -> Optional[PRReview]: print(f"WARNING: Could not find review file ('{review_file}')", flush=True) return None - with open(review_file) as review_file_handle: + with review_file.open() as review_file_handle: payload = json.load(review_file_handle) return payload or None def load_and_merge_profiling() -> Dict: result = {} - for profile_file in glob.iglob(os.path.join(PROFILE_DIR, "*.json")): - profile_dict = json.load(open(profile_file)) + for profile_file in Path(PROFILE_DIR).glob("*.json"): + profile_dict = json.load(profile_file.open()) filename = profile_dict["file"] current_profile = result.get(filename, {}) for check, timing in profile_dict["profile"].items(): @@ -1150,7 +1149,7 @@ def get_line_ranges(diff, files): # Adding a copy of the line filters with backslashes allows for both cl.exe and clang.exe to work. if os.path.sep == "\\": # Converts name to backslashes for the cl.exe line filter. - name = os.path.join(*name.split("/")) + name = Path.joinpath(*name.split("/")) line_filter_json.append({"name": name, "lines": lines}) return json.dumps(line_filter_json, separators=(",", ":")) @@ -1196,7 +1195,7 @@ def set_output(key: str, val: str) -> bool: return False # append key-val pair to file - with open(os.environ["GITHUB_OUTPUT"], "a") as f: + with Path(os.environ["GITHUB_OUTPUT"]).open("a") as f: f.write(f"{key}={val}\n") return True @@ -1207,7 +1206,7 @@ def set_summary(val: str) -> bool: return False # append key-val pair to file - with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as f: + with Path(os.environ["GITHUB_STEP_SUMMARY"]).open("a") as f: f.write(val) return True diff --git a/post/clang_tidy_review/clang_tidy_review/review.py b/post/clang_tidy_review/clang_tidy_review/review.py index 379d8c7..c186736 100755 --- a/post/clang_tidy_review/clang_tidy_review/review.py +++ b/post/clang_tidy_review/clang_tidy_review/review.py @@ -6,10 +6,9 @@ # See LICENSE for more information import argparse -import os -import pathlib import re import subprocess +from pathlib import Path from clang_tidy_review import ( PullRequest, @@ -39,7 +38,7 @@ def main(): "--clang_tidy_binary", help="clang-tidy binary", default="clang-tidy-14", - type=pathlib.Path, + type=Path, ) parser.add_argument( "--build_dir", help="Directory with compile_commands.json", default="." @@ -145,7 +144,7 @@ def main(): with message_group(f"Running cmake: {cmake_command}"): subprocess.run(cmake_command, shell=True, check=True) - elif os.path.exists(build_compile_commands): + elif Path(build_compile_commands).exists(): fix_absolute_paths(build_compile_commands, args.base_dir) pull_request = PullRequest(args.repo, args.pr, get_auth_from_arguments(args)) From 71fa1583db3d7f4ec226ef3d6c87b57be3a687e1 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 1 Nov 2024 19:48:27 +0100 Subject: [PATCH 2/4] fix: merge `with` (`SIM117`) --- .../clang_tidy_review/__init__.py | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 0232b57..775327f 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -929,32 +929,34 @@ def create_review( files = filter_files(diff, include, exclude) if files == []: - with message_group("No files to check!"): - with Path(REVIEW_FILE).open("w") as review_file: - json.dump( - { - "body": "clang-tidy found no files to check", - "event": "COMMENT", - "comments": [], - }, - review_file, - ) + with message_group("No files to check!"), Path(REVIEW_FILE).open( + "w" + ) as review_file: + json.dump( + { + "body": "clang-tidy found no files to check", + "event": "COMMENT", + "comments": [], + }, + review_file, + ) return None print(f"Checking these files: {files}", flush=True) line_ranges = get_line_ranges(diff, files) if line_ranges == "[]": - with message_group("No lines added in this PR!"): - with Path(REVIEW_FILE).open("w") as review_file: - json.dump( - { - "body": "clang-tidy found no lines added", - "event": "COMMENT", - "comments": [], - }, - review_file, - ) + with message_group("No lines added in this PR!"), Path(REVIEW_FILE).open( + "w" + ) as review_file: + json.dump( + { + "body": "clang-tidy found no lines added", + "event": "COMMENT", + "comments": [], + }, + review_file, + ) return None print(f"Line filter for clang-tidy:\n{line_ranges}\n") From 5e9fb02cd82bf6494d4beb0b131af93e0d3e1fa3 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 1 Nov 2024 19:49:45 +0100 Subject: [PATCH 3/4] fix: use positional args for regex (`B034`) --- post/clang_tidy_review/clang_tidy_review/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 775327f..303ff7b 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -1224,7 +1224,7 @@ def decorate_check_names(comment: str) -> str: url = f"https://clang.llvm.org/{version}/clang-tidy/checks" regex = r"(\[((?:clang-analyzer)|(?:(?!clang)[\w]+))-([\.\w-]+)\]$)" subst = f"[\\g<1>({url}/\\g<2>/\\g<3>.html)]" - return re.sub(regex, subst, comment, 1, re.MULTILINE) + return re.sub(regex, subst, comment, count=1, flags=re.MULTILINE) def decorate_comment(comment: PRReviewComment) -> PRReviewComment: From 87a563ce94ff56fa79e055f3d503352b1b2c14e7 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Fri, 1 Nov 2024 20:31:57 +0100 Subject: [PATCH 4/4] fix: correct various types --- .../clang_tidy_review/__init__.py | 33 ++++++++----------- .../clang_tidy_review/post.py | 2 +- .../clang_tidy_review/review.py | 2 +- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/post/clang_tidy_review/clang_tidy_review/__init__.py b/post/clang_tidy_review/clang_tidy_review/__init__.py index 303ff7b..154e951 100644 --- a/post/clang_tidy_review/clang_tidy_review/__init__.py +++ b/post/clang_tidy_review/clang_tidy_review/__init__.py @@ -19,13 +19,14 @@ import zipfile from operator import itemgetter from pathlib import Path -from typing import Dict, List, Optional, TypedDict +from typing import Any, Dict, List, Optional, TypedDict import unidiff import urllib3 import yaml from github import Auth, Github from github.PaginatedList import PaginatedList +from github.PullRequest import ReviewComment from github.Requester import Requester from github.WorkflowRun import WorkflowRun @@ -46,20 +47,10 @@ class Metadata(TypedDict): pr_number: int -class PRReviewComment(TypedDict): - path: str - position: Optional[int] - body: str - line: Optional[int] - side: Optional[str] - start_line: Optional[int] - start_side: Optional[str] - - class PRReview(TypedDict): body: str event: str - comments: List[PRReviewComment] + comments: list[ReviewComment] class HashableComment: @@ -125,7 +116,7 @@ def add_auth_arguments(parser: argparse.ArgumentParser): group_app.add_argument("--installation-id", type=int, help="app installation ID") -def get_auth_from_arguments(args: argparse.Namespace) -> Auth: +def get_auth_from_arguments(args: argparse.Namespace) -> Auth.Auth: if args.token: return Auth.Token(args.token) @@ -260,7 +251,7 @@ def load_clang_tidy_warnings(): class PullRequest: """Add some convenience functions not in PyGithub""" - def __init__(self, repo: str, pr_number: Optional[int], auth: Auth) -> None: + def __init__(self, repo: str, pr_number: Optional[int], auth: Auth.Auth) -> None: self.repo_name = repo self.pr_number = pr_number self.auth = auth @@ -539,7 +530,7 @@ def replace_one_line(replacement_set, line_num, offset_lookup): line_offset = offset_lookup[filename][line_num] # List of (start, end) offsets from line_offset - insert_offsets = [(0, 0)] + insert_offsets: list[tuple[Optional[int], Optional[int]]] = [(0, 0)] # Read all the source lines into a dict so we only get one copy of # each line, though we might read the same line in multiple times source_lines = {} @@ -755,7 +746,7 @@ def create_review_file( if "Diagnostics" not in clang_tidy_warnings: return None - comments: List[PRReviewComment] = [] + comments: List[ReviewComment] = [] for diagnostic in clang_tidy_warnings["Diagnostics"]: try: @@ -1227,7 +1218,7 @@ def decorate_check_names(comment: str) -> str: return re.sub(regex, subst, comment, count=1, flags=re.MULTILINE) -def decorate_comment(comment: PRReviewComment) -> PRReviewComment: +def decorate_comment(comment: ReviewComment) -> ReviewComment: comment["body"] = decorate_check_names(comment["body"]) return comment @@ -1288,10 +1279,12 @@ def convert_comment_to_annotations(comment): } -def post_annotations(pull_request: PullRequest, review: Optional[PRReview]) -> int: +def post_annotations( + pull_request: PullRequest, review: Optional[PRReview] +) -> Optional[int]: """Post the first 10 comments in the review as annotations""" - body = { + body: dict[str, Any] = { "name": "clang-tidy-review", "head_sha": pull_request.pull_request.head.sha, "status": "completed", @@ -1309,7 +1302,7 @@ def post_annotations(pull_request: PullRequest, review: Optional[PRReview]) -> i for comment in review["comments"]: first_line = comment["body"].splitlines()[0] comments.append( - f"{comment['path']}:{comment.get('start_line', comment['line'])}: {first_line}" + f"{comment['path']}:{comment.get('start_line', comment.get('line', 0))}: {first_line}" ) total_comments = len(review["comments"]) diff --git a/post/clang_tidy_review/clang_tidy_review/post.py b/post/clang_tidy_review/clang_tidy_review/post.py index 4c337f2..67ff296 100755 --- a/post/clang_tidy_review/clang_tidy_review/post.py +++ b/post/clang_tidy_review/clang_tidy_review/post.py @@ -105,7 +105,7 @@ def main() -> int: pull_request, review, args.max_comments, lgtm_comment_body, args.dry_run ) - return exit_code if args.num_comments_as_exitcode else 0 + return (exit_code or 0) if args.num_comments_as_exitcode else 0 if __name__ == "__main__": diff --git a/post/clang_tidy_review/clang_tidy_review/review.py b/post/clang_tidy_review/clang_tidy_review/review.py index c186736..9efb8e6 100755 --- a/post/clang_tidy_review/clang_tidy_review/review.py +++ b/post/clang_tidy_review/clang_tidy_review/review.py @@ -164,7 +164,7 @@ def main(): if args.split_workflow: total_comments = 0 if review is None else len(review["comments"]) - set_output("total_comments", total_comments) + set_output("total_comments", str(total_comments)) print("split_workflow is enabled, not posting review") return