From 5cb47befe3ab64b156ab4d2aa8b570be87f0c550 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Franch?= Date: Wed, 7 Oct 2026 17:04:00 +0200 Subject: [PATCH] ci(conventions): allow comments with a TechnicalDebt change, reject non-text files --- CONTRIBUTING.md | 2 +- tools/check_conventions.py | 21 ++++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f36977c5e..a901ee126 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,7 +26,7 @@ ctest --test-dir build --output-on-failure Python 3 is optional; without it some relinker tests are not registered. -The Conventions check runs on every pull request and fails when a rule on this page is broken. Run it locally before pushing: +The Conventions check runs on every pull request and fails when a rule on this page is broken. It accepts code comments only when the pull request also changes [TechnicalDebt](docs/dev/TechnicalDebt.md), and only UTF-8 text files. Run it locally before pushing: ``` python3 tools/check_conventions.py --base origin/main diff --git a/tools/check_conventions.py b/tools/check_conventions.py index 2a2a96e1e..8d0de9a8d 100644 --- a/tools/check_conventions.py +++ b/tools/check_conventions.py @@ -7,14 +7,14 @@ from collections import defaultdict from pathlib import PurePosixPath RULES = { - "comment": ("error", "docs/dev/CONVENTIONS.md", "Comments are only for technical debt, the end of #endif and the end of a namespace"), + "comment": ("error", "docs/dev/CONVENTIONS.md", "Comments are only for technical debt, so they need a change to docs/dev/TechnicalDebt.md in the same pull request; #endif and namespace ends are always allowed"), "silent-stub": ("error", "CONTRIBUTING.md#code", "Unimplemented exports call NotImplemented_nid_no_patch(__func__); silent stubs are listed in docs/dev/TechnicalDebt.md#silent-stubs"), "duplicate-export": ("error", "CONTRIBUTING.md#code", "Already exported by another library: remove the copy, or use APS5_DUMMY_FUN if nothing is left"), "system-dependency": ("error", "CONTRIBUTING.md#code", "Third-party code is a submodule under 3rdparty/ built from source, not found on the system"), "title-specific": ("error", "CONTRIBUTING.md#code", "Implement the general behaviour, not what one title needs; title-specific code belongs in its sce_module"), "extension": ("error", "CONTRIBUTING.md#code", "Avoid non-standard extensions where standard C++ is enough"), "notes-file": ("error", "CONTRIBUTING.md#branches-and-pull-requests", "Notes, investigation and agent files go in the pull request, not in the repository"), - "binary": ("error", "docs/dev/CONVENTIONS.md", "No images or binary files in the repository; images go in the gist comments"), + "binary": ("error", "docs/dev/CONVENTIONS.md", "Only UTF-8 text files in the repository; images go in the gist comments"), "doc-link": ("error", "CONTRIBUTING.md#documentation", "Relative link to a file that does not exist"), "commit-subject": ("error", "docs/dev/CONVENTIONS.md", "Commit subject is not Conventional Commits"), "pr-title": ("error", "docs/dev/CONVENTIONS.md", "Pull request title is not Conventional Commits"), @@ -39,8 +39,9 @@ LINK = re.compile(r"\]\(([^)\s]+)") SECTION = re.compile(r"^###\s*(.+?)\s*$", re.M) -def git(*args): - return subprocess.run(["git", *args], capture_output=True, text=True, check=True).stdout +def git(*args, text=True): + output = subprocess.run(["git", *args], capture_output=True, check=True).stdout + return output.decode("utf-8", errors="replace") if text else output class Check: @@ -106,6 +107,8 @@ class Check: for number, text in lines: if "#" in STRINGS.sub('""', text) and not text.startswith("#!"): self.report("comment", path, number) + if "docs/dev/TechnicalDebt.md" in self.status: + self.findings = [finding for finding in self.findings if finding[0] != "comment"] def duplicates(self, exports): if not exports: @@ -134,7 +137,7 @@ class Check: if path.startswith("3rdparty/"): continue suffix = PurePosixPath(path).suffix.lower() - if self.status.get(path) != "D" and (added == "-" or suffix in IMAGES): + if self.status.get(path) != "D" and (added == "-" or suffix in IMAGES or not utf8(git("show", f"{self.head}:{path}", text=False))): self.report("binary", path, 0) if self.status.get(path) == "A" and allowed_notes(path) is False: self.report("notes-file", path, 0) @@ -173,6 +176,14 @@ class Check: self.report("pr-template", "", 0, "fill Depends on with #N or none") +def utf8(data): + try: + data.decode("utf-8") + except UnicodeDecodeError: + return False + return True + + def library(path): return PurePosixPath(path).parts[3]