diff --git a/scripts/review_pr.py b/scripts/review_pr.py index a533790..26c5211 100644 --- a/scripts/review_pr.py +++ b/scripts/review_pr.py @@ -177,16 +177,37 @@ 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: + old_match = ENTRY_RE.match(old_line) + new_match = ENTRY_RE.match(new_line) + if old_match is None or new_match is None: + return False + + if normalize(old_match.group(1)) == normalize(new_match.group(1)): + return True + + old_urls = { + canonicalize_url(url) for url in MARKDOWN_URL_RE.findall(old_line) + } + new_urls = { + canonicalize_url(url) for url in MARKDOWN_URL_RE.findall(new_line) + } + return bool(old_urls & new_urls) + + def analyze_readme_change( base_readme: str, head_readme: str, -) -> tuple[str | None, list[Finding]]: +) -> tuple[str | None, str | None, list[Finding]]: added_lines, removed_lines = readme_changed_lines(base_readme, head_readme) substantive_added = [line for line in added_lines if line.strip()] substantive_removed = [line for line in removed_lines if line.strip()] entry_lines = [ line for line in substantive_added if line.strip().startswith("- ") ] + removed_entry_lines = [ + line for line in substantive_removed if line.strip().startswith("- ") + ] findings: list[Finding] = [] if len(entry_lines) != 1: findings.append( @@ -195,19 +216,39 @@ def analyze_readme_change( "expected exactly one added README entry line", ) ) - return None, findings + return None, None, findings unauthorized_additions = [ line for line in substantive_added if line != entry_lines[0] ] - if substantive_removed or unauthorized_additions: + unauthorized_removals = [ + line for line in substantive_removed if line not in removed_entry_lines + ] + removed_entry_line = ( + removed_entry_lines[0] if len(removed_entry_lines) == 1 else None + ) + invalid_update = bool(substantive_removed) and ( + removed_entry_line is None + or bool(unauthorized_removals) + or not entries_represent_same_project(removed_entry_line, entry_lines[0]) + ) + if unauthorized_additions or invalid_update: findings.append( Finding( "content", - "README changes must add one entry without other substantive edits", + "README changes must add one entry or update the same project " + "without other substantive edits", ) ) - return entry_lines[0], findings + return entry_lines[0], removed_entry_line, findings + + +def remove_entry_line(readme_text: str, entry_line: str | None) -> str: + if entry_line is None: + return readme_text + lines = readme_text.splitlines() + lines.remove(entry_line) + return "\n".join(lines) def readme_changed_lines( @@ -474,7 +515,10 @@ def review_pr( base_readme = read_readme(repository, pull_request.base.sha) head_readme = read_readme(repository, pull_request.head.sha) - entry_line, change_findings = analyze_readme_change(base_readme, head_readme) + entry_line, removed_entry_line, change_findings = analyze_readme_change( + base_readme, + head_readme, + ) findings.extend(change_findings) if entry_line is None: return findings, pull_request.title @@ -545,9 +589,13 @@ def review_pr( github_urls.extend(GITHUB_LINK_RE.findall(line)) github_urls = list(dict.fromkeys(github_urls)) if not github_urls: - findings.append( - Finding("github", "no GitHub repository URL found; cannot verify activity") - ) + if section != "Commercial & Proprietary Services": + findings.append( + Finding( + "github", + "no GitHub repository URL found; cannot verify activity", + ) + ) else: repository_parts = parse_github_repository_url(github_urls[0]) if not repository_parts: @@ -587,7 +635,8 @@ def review_pr( if not primary_github and not url_reachable(url): findings.append(Finding("reachability", f"primary URL is not reachable: {url}")) - if readme_has_duplicate(base_readme, name, [url, *github_urls]): + duplicate_base_readme = remove_entry_line(base_readme, removed_entry_line) + if readme_has_duplicate(duplicate_base_readme, name, [url, *github_urls]): findings.append( Finding("duplicates", "project name or URL already exists in README.md") ) diff --git a/tests/test_review_pr.py b/tests/test_review_pr.py index 25396af..48f7959 100644 --- a/tests/test_review_pr.py +++ b/tests/test_review_pr.py @@ -662,6 +662,90 @@ class ValidationPipelineTests(unittest.TestCase): ), ) + def test_accepts_description_update_for_the_same_project(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n" + ) + head_readme = base_readme.replace("Old description.", "New description.") + + self.assertEqual( + self.review(base_readme=base_readme, head_readme=head_readme), + set(), + ) + + def test_accepts_tag_update_for_the_same_project(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n" + ) + head_readme = base_readme.replace("`Python`", "`Python` `Rust`") + + self.assertEqual( + self.review(base_readme=base_readme, head_readme=head_readme), + set(), + ) + + def test_accepts_url_update_when_the_name_is_unchanged(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Fresh](https://old.example.com/fresh) - `Python` - Fresh project. " + "[GitHub](https://github.com/example/fresh)\n" + ) + head_readme = base_readme.replace( + "https://old.example.com/fresh", + "https://new.example.com/fresh", + ) + + self.assertEqual( + self.review(base_readme=base_readme, head_readme=head_readme), + set(), + ) + + def test_accepts_rename_when_the_repository_is_unchanged(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Old Name](https://github.com/example/fresh) - `Python` - Fresh project.\n" + ) + head_readme = base_readme.replace("[Old Name]", "[Fresh]") + + self.assertEqual( + self.review(base_readme=base_readme, head_readme=head_readme), + set(), + ) + + def test_accepts_section_move_for_the_same_project(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n" + "\n## Portfolio Optimization & Risk Analysis\n" + ) + head_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "\n## Portfolio Optimization & Risk Analysis\n" + "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n" + ) + + self.assertEqual( + self.review(base_readme=base_readme, head_readme=head_readme), + set(), + ) + + def test_rejects_replacing_an_entry_with_an_unrelated_project(self): + base_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Old](https://github.com/example/old) - `Python` - Old project.\n" + ) + head_readme = ( + "# awesome-quant\n\n## Trading & Backtesting\n" + "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n" + ) + + self.assertIn( + "content", + self.review(base_readme=base_readme, head_readme=head_readme), + ) + def test_rejects_deleting_an_existing_entry(self): base_readme = ( "# awesome-quant\n\n" @@ -790,6 +874,19 @@ class ValidationPipelineTests(unittest.TestCase): ) self.assertIn("github-link", self.review(patch_text=patch_text)) + def test_accepts_commercial_entry_without_github_repository(self): + patch_text = """@@ -1,1 +1,2 @@ + ## Commercial & Proprietary Services ++- [Fresh](https://example.com/fresh) - Commercial risk calculator. +""" + + base_readme = "# awesome-quant\n\n## Commercial & Proprietary Services\n" + + self.assertEqual( + self.review(patch_text=patch_text, base_readme=base_readme), + set(), + ) + def test_rejects_entry_without_github_repository(self): patch_text = VALID_PATCH.replace( "https://github.com/example/fresh",