From bf1c1f37857f8f10fabc3dbcb491abc697861beb Mon Sep 17 00:00:00 2001 From: Wilson Freitas Date: Sat, 12 Sep 2026 22:00:51 -0300 Subject: [PATCH] Recognize canonical repository renames in README maintenance (#663) --- scripts/review_pr.py | 62 +++++++++++++++++++++++++++--- tests/test_review_pr.py | 85 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 6 deletions(-) diff --git a/scripts/review_pr.py b/scripts/review_pr.py index aa4ecff..815577f 100644 --- a/scripts/review_pr.py +++ b/scripts/review_pr.py @@ -14,6 +14,7 @@ import ssl import sys from dataclasses import dataclass from datetime import datetime, timedelta, timezone +from functools import cache from pathlib import Path from typing import Any, Callable from urllib.parse import urlsplit, urlunsplit @@ -180,7 +181,20 @@ def read_readme(repository: Any, ref: str) -> str: return content.decoded_content.decode("utf-8") -def entries_represent_same_project(old_line: str, new_line: str) -> bool: +def entry_github_urls(line: str) -> set[str]: + match = ENTRY_RE.match(line) + if match is None: + return set() + urls = set(GITHUB_LINK_RE.findall(line)) + if parse_github_repository_url(match.group(2).strip()): + urls.add(match.group(2).strip()) + return urls + + +def entries_represent_same_project( + old_line: str, new_line: str, + repository_id: Callable[[str], int | None] | None = None, +) -> bool: old_match = ENTRY_RE.match(old_line) new_match = ENTRY_RE.match(new_line) if old_match is None or new_match is None: @@ -195,11 +209,18 @@ def entries_represent_same_project(old_line: str, new_line: str) -> bool: new_urls = { canonicalize_url(url) for url in MARKDOWN_URL_RE.findall(new_line) } - return bool(old_urls & new_urls) + if old_urls & new_urls: + return True + if repository_id is None: + return False + old_ids = {repository_id(url) for url in entry_github_urls(old_line)} - {None} + new_ids = {repository_id(url) for url in entry_github_urls(new_line)} - {None} + return bool(old_ids & new_ids) def analyze_readme_change( patch: str | None, + repository_id: Callable[[str], int | None] | None = None, ) -> tuple[list[str], list[str], list[Finding]]: added_lines: list[str] = [] removed_lines: list[str] = [] @@ -219,7 +240,9 @@ def analyze_readme_change( line for line in substantive_removed if line.strip().startswith("- ") ] findings: list[Finding] = [] - previous_entries = match_existing_entries(entry_lines, removed_entry_lines) + previous_entries = match_existing_entries( + entry_lines, removed_entry_lines, repository_id + ) maintenance = bool(removed_entry_lines) and all( previous is not None for previous in previous_entries ) @@ -259,13 +282,17 @@ def analyze_readme_change( def match_existing_entries( added: list[str], removed: list[str], + repository_id: Callable[[str], int | None] | None = None, ) -> list[str | None]: """Pair updates one-to-one; an extra copy is still a new entry.""" unmatched = list(removed) previous_entries: list[str | None] = [] for line in added: index = next( - (i for i, old in enumerate(unmatched) if entries_represent_same_project(old, line)), + ( + i for i, old in enumerate(unmatched) + if entries_represent_same_project(old, line, repository_id) + ), None, ) previous_entries.append(unmatched.pop(index) if index is not None else None) @@ -534,6 +561,7 @@ def review_entry( readme_cache: dict[str, str], previous_line: str | None = None, previous_section: str = "", + repository_id: Callable[[str], int | None] | None = None, ) -> list[Finding]: findings: list[Finding] = [] match = ENTRY_RE.match(line) @@ -629,6 +657,13 @@ def review_entry( canonicalize_url(item) for item in github_urls } == {canonicalize_url(item) for item in previous_github_urls} + if same_section_update and not unchanged_source and repository_id is not None: + previous_ids = {repository_id(item) for item in previous_github_urls} + current_ids = {repository_id(item) for item in github_urls} + unchanged_source = bool(current_ids) and None not in current_ids and ( + current_ids == previous_ids + ) + if not github_urls: if section != "Commercial & Proprietary Services" and not unchanged_source: findings.append( @@ -731,10 +766,22 @@ def review_pr( ) return findings, pull_request.title, 0 + @cache + def repository_id(url: str) -> int | None: + parts = parse_github_repository_url(url) + if parts is None: + return None + try: + identity = getattr(client.get_repo("/".join(parts)), "id", None) + except GithubException: + # Unverified identities cannot grant a cleanup exemption. + return None + return identity if type(identity) is int and identity > 0 else None + base_readme = read_readme(repository, pull_request.base.sha) head_readme = read_readme(repository, pull_request.head.sha) entry_lines, removed_entry_lines, change_findings = analyze_readme_change( - files[0].patch, + files[0].patch, repository_id, ) findings.extend(change_findings) if not entry_lines: @@ -745,7 +792,9 @@ def review_pr( pull_request.base.sha: base_readme, pull_request.head.sha: head_readme, } - previous_entries = match_existing_entries(entry_lines, removed_entry_lines) + previous_entries = match_existing_entries( + entry_lines, removed_entry_lines, repository_id + ) maintenance = bool(removed_entry_lines) and all( previous is not None for previous in previous_entries ) @@ -764,6 +813,7 @@ def review_pr( check_pull_request_duplicates=check_pull_request_duplicates, readme_cache=readme_cache, previous_line=previous_line, + repository_id=repository_id, previous_section=( find_entry_section(base_readme, previous_line, require_unique=False) if previous_line diff --git a/tests/test_review_pr.py b/tests/test_review_pr.py index ba42668..d66504e 100644 --- a/tests/test_review_pr.py +++ b/tests/test_review_pr.py @@ -13,6 +13,7 @@ from scripts.review_pr import ( Finding, PinnedHTTPSConnection, main, + match_existing_entries, readme_has_duplicate, review_pr, url_reachable, @@ -620,6 +621,90 @@ class ValidationPipelineTests(unittest.TestCase): old, old.replace("Old", "New"), configure_project=missing_docs )) + def test_cleanup_same_id_rename_preserves_legacy_exemption(self): + old = "- [Fresh](https://github.com/example/old) - `Python` - Research tool.\n" + def legacy(project): + project.id = 123 + project.archived = True + project.pushed_at = NOW - timedelta(days=800) + self.assertEqual(self.review_cleanup( + old, old.replace("example/old", "example/fresh"), configure_project=legacy + ), set()) + + def test_cleanup_six_name_and_url_renames_match_same_ids(self): + old = "".join( + f"- [Old {i}](https://github.com/example/old-{i}) - `Python` - Tool.\n" + for i in range(6) + ) + new = old.replace("[Old", "[New").replace("/old-", "/new-") + original = FakeClient.get_repo + requested = [] + def lookup(client, name): + requested.append(name) + project = original(client, name) + if name.startswith("example/"): + project.id = int(name.rsplit("-", 1)[1]) + 1 + return project + with patch.object(FakeClient, "get_repo", lookup): + self.assertEqual(self.review_cleanup(old, new), set()) + for i in range(6): + self.assertEqual(requested.count(f"example/old-{i}"), 1) + + def test_cleanup_rename_does_not_pair_extra_copy(self): + old = "- [Old](https://github.com/example/old) - `Python` - Tool.\n" + new = old.replace("[Old]", "[New]").replace("/old)", "/new)") + def identity(project): + project.id = 123 + self.assertEqual( + match_existing_entries([new, new], [old], lambda url: 123), + [old, None], + ) + self.assertIn("duplicates", self.review_cleanup( + old, new + new.replace("Tool.", "Second copy."), configure_project=identity + )) + + def test_cleanup_different_ids_still_check_activity(self): + old = "- [Fresh](https://github.com/example/old) - `Python` - Tool.\n" + original = FakeClient.get_repo + def lookup(client, name): + project = original(client, name) + if name.startswith("example/"): + project.id = 1 if name.endswith("/old") else 2 + project.pushed_at = NOW - timedelta(days=800) + return project + with patch.object(FakeClient, "get_repo", lookup): + self.assertIn("activity", self.review_cleanup( + old, old.replace("/old)", "/new)") + )) + + def test_cleanup_same_id_rename_section_move_checks_activity(self): + old = "- [Old](https://github.com/example/old) - `Python` - Tool.\n" + new = old.replace("[Old]", "[New]").replace("/old)", "/new)") + def archived(project): + project.id = 123 + project.archived = True + self.assertIn("activity", self.review_cleanup( + old + "\n## Portfolio Optimization & Risk Analysis\n", + "\n## Portfolio Optimization & Risk Analysis\n" + new, + configure_project=archived, + )) + + def test_cleanup_failed_old_identity_lookup_does_not_grant_exemption(self): + old = "- [Fresh](https://github.com/example/old) - `Python` - Tool.\n" + original = FakeClient.get_repo + def lookup(client, name): + if name == "example/old": + raise GithubException(404, {"message": "Not Found"}) + project = original(client, name) + if name.startswith("example/"): + project.id = 123 + project.pushed_at = NOW - timedelta(days=800) + return project + with patch.object(FakeClient, "get_repo", lookup): + self.assertIn("activity", self.review_cleanup( + old, old.replace("/old)", "/new)") + )) + def test_cleanup_rechecks_replacement_repository_activity(self): old = "- [Fresh](https://github.com/example/old) - `Python` - Old description.\n" def stale(project):