-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add check-repo-links hook to verify @repo relative links #163
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
base: master
Are you sure you want to change the base?
Changes from all commits
28bb1d3
62b0c6c
624a4bf
bee5d8f
315ef7e
70fac0c
be234c3
f069b44
970035d
a85518d
be89604
c3f27fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,147 @@ | ||||||
| # Copyright (c) Luminar Technologies, Inc. All rights reserved. | ||||||
| # Licensed under the MIT License. | ||||||
|
|
||||||
| """Verify that ``@repo`` relative links in text files point to existing files or directories.""" | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
|
||||||
| import posixpath | ||||||
| import re | ||||||
| import subprocess | ||||||
| import sys | ||||||
| from pathlib import Path | ||||||
| from typing import TYPE_CHECKING, NamedTuple | ||||||
|
|
||||||
| from dev_tools.utils.git_hook_utils import parse_arguments | ||||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
| from collections.abc import Callable, Iterable, Sequence | ||||||
|
|
||||||
| # The marker must not be preceded by a word character or another ``@`` (so ``@report``, ``foo@repo`` | ||||||
| # and ``@@repo`` never match) and must be followed by same-line whitespace and a non-whitespace path. | ||||||
| # Requiring whitespace (rather than any delimiter) avoids the common npm/pnpm scope ``@repo/...``. | ||||||
| MARKER = re.compile(rb"(?<![\w@])@repo[ \t]+(\S+)") | ||||||
|
|
||||||
| # Trailing punctuation that is almost never part of a real path and usually comes from prose or | ||||||
| # enclosing syntax, e.g. "see @repo foo.md." or "(@repo foo.md)". | ||||||
| _TRAILING_PUNCTUATION = ".,;:!?)\"'`]}>" | ||||||
|
|
||||||
|
|
||||||
| class BrokenLink(NamedTuple): | ||||||
| """A relative ``@repo`` link that does not resolve to a tracked file or directory.""" | ||||||
|
|
||||||
| file_path: str | ||||||
| line: int | ||||||
| column: int | ||||||
| link: str | ||||||
|
|
||||||
|
|
||||||
| def resolve_link_target(link: str, file_path: str) -> str | None: | ||||||
| """Resolve a raw ``@repo`` link to a repo-root-relative path, or None if it should be skipped. | ||||||
|
|
||||||
| Links starting with ``/`` are resolved from the repository root, everything else relative to the | ||||||
| file the link appears in. URLs, mailto links and anchor-only targets are skipped. | ||||||
| """ | ||||||
| link = link.split("#", 1)[0].split("?", 1)[0].rstrip(_TRAILING_PUNCTUATION) | ||||||
| if not link or "://" in link or link.startswith("mailto:"): | ||||||
| return None | ||||||
| if link.startswith("/"): | ||||||
| return posixpath.normpath(link[1:]) | ||||||
| return posixpath.normpath(posixpath.join(posixpath.dirname(file_path), link)) | ||||||
|
|
||||||
|
|
||||||
| def offset_to_line_and_column(content: bytes, offset: int) -> tuple[int, int]: | ||||||
| """Return the 1-based (line, column) of a byte offset within content.""" | ||||||
| line = content.count(b"\n", 0, offset) + 1 | ||||||
| column = offset - content.rfind(b"\n", 0, offset) | ||||||
| return line, column | ||||||
|
|
||||||
|
|
||||||
| def find_broken_links_in_content( | ||||||
| content: bytes, | ||||||
| file_path: str, | ||||||
| does_target_exist: Callable[[str], bool], | ||||||
| ) -> list[BrokenLink]: | ||||||
| """Scan a single file's bytes and return the broken ``@repo`` links it contains.""" | ||||||
| # Fast pre-filter: skip the overwhelming majority of files without running the regex. | ||||||
| if b"@repo" not in content: | ||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we
Suggested change
instead?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept |
||||||
| return [] | ||||||
|
Comment on lines
+67
to
+68
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's avoid premature optimization: benchmark the hook without this pre-filter on 10k files with 100line each, and 1% of the files contain
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Benchmarked as requested (10k files x 100 lines, 1% contain |
||||||
| broken_links: list[BrokenLink] = [] | ||||||
| for match in MARKER.finditer(content): | ||||||
| link = match.group(1).decode("utf-8", errors="replace") | ||||||
| target = resolve_link_target(link, file_path) | ||||||
| if target is None or does_target_exist(target): | ||||||
| continue | ||||||
| # Only for a confirmed broken link do we pay for computing the precise location. | ||||||
| line, column = offset_to_line_and_column(content, match.start(1)) | ||||||
| broken_links.append(BrokenLink(file_path, line, column, link)) | ||||||
| return broken_links | ||||||
|
|
||||||
|
|
||||||
| def find_broken_links( | ||||||
| files_to_check: list[Path], | ||||||
| repository_root: Path, | ||||||
| does_target_exist: Callable[[str], bool], | ||||||
| ) -> list[BrokenLink]: | ||||||
| broken_links: list[BrokenLink] = [] | ||||||
| for file in files_to_check: | ||||||
| repo_relative_path = file.resolve().relative_to(repository_root).as_posix() | ||||||
| broken_links.extend(find_broken_links_in_content(file.read_bytes(), repo_relative_path, does_target_exist)) | ||||||
| return broken_links | ||||||
|
|
||||||
|
|
||||||
| def build_set_of_valid_link_targets(tracked_files: Iterable[str]) -> set[str]: | ||||||
| """Build the set of valid link targets: every tracked file plus all of their parent directories.""" | ||||||
| valid_targets = set(tracked_files) | ||||||
| for path in list(valid_targets): | ||||||
| parent = posixpath.dirname(path) | ||||||
| while parent: | ||||||
| valid_targets.add(parent) | ||||||
| parent = posixpath.dirname(parent) | ||||||
| valid_targets.add(".") # the repository root itself | ||||||
| return valid_targets | ||||||
|
|
||||||
|
|
||||||
| def build_target_existence_check(tracked_files: Iterable[str]) -> Callable[[str], bool]: | ||||||
| """Return a predicate reporting whether a link target is a tracked file or directory.""" | ||||||
| return build_set_of_valid_link_targets(tracked_files).__contains__ | ||||||
|
|
||||||
|
|
||||||
| def get_repository_root() -> Path: | ||||||
| output = subprocess.run( | ||||||
| ["git", "rev-parse", "--show-toplevel"], # noqa: S607 | ||||||
| capture_output=True, | ||||||
| check=True, | ||||||
| text=True, | ||||||
| ).stdout | ||||||
| return Path(output.strip()) | ||||||
|
|
||||||
|
|
||||||
| def list_tracked_files(repository_root: Path) -> list[str]: | ||||||
| output = subprocess.run( | ||||||
| ["git", "-C", str(repository_root), "ls-files", "-z"], # noqa: S607 | ||||||
| capture_output=True, | ||||||
| check=True, | ||||||
| ).stdout | ||||||
| return [path.decode("utf-8", "surrogateescape") for path in output.split(b"\0") if path] | ||||||
|
|
||||||
|
|
||||||
| def print_broken_links(broken_links: list[BrokenLink]) -> None: | ||||||
| if not broken_links: | ||||||
| return | ||||||
| print("Found broken links:") | ||||||
| for link in broken_links: | ||||||
| print(f"{link.file_path}:{link.line}:{link.column} {link.link}") | ||||||
|
|
||||||
|
|
||||||
| def main(argv: Sequence[str] | None = None) -> int: | ||||||
| files = parse_arguments(argv).filenames | ||||||
| repository_root = get_repository_root() | ||||||
| does_target_exist = build_target_existence_check(list_tracked_files(repository_root)) | ||||||
| broken_links = find_broken_links(files, repository_root, does_target_exist) | ||||||
| print_broken_links(broken_links) | ||||||
| return 1 if broken_links else 0 | ||||||
|
|
||||||
|
|
||||||
| if __name__ == "__main__": | ||||||
| sys.exit(main()) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| # Copyright (c) Luminar Technologies, Inc. All rights reserved. | ||
| # Licensed under the MIT License. | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
|
|
||
| from dev_tools.check_repo_links import ( | ||
| BrokenLink, | ||
| build_set_of_valid_link_targets, | ||
| build_target_existence_check, | ||
| find_broken_links_in_content, | ||
| main, | ||
| offset_to_line_and_column, | ||
| resolve_link_target, | ||
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pyfakefs.fake_filesystem import FakeFilesystem | ||
|
|
||
| TRACKED = ["README.md", "docs/readme.md", "src/foo.hpp", "src/util/helper.h", "src/c.h"] | ||
|
|
||
|
|
||
| def scan(text: str, file_path: str = "src/foo.cpp", *, tracked: tuple[str, ...] = ()) -> list[BrokenLink]: | ||
| return find_broken_links_in_content(text.encode(), file_path, build_target_existence_check(tracked)) | ||
|
|
||
|
|
||
| # --- resolve_link_target -------------------------------------------------------------------- | ||
|
|
||
|
Comment on lines
+31
to
+32
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove all such comments. They just duplicate the test name |
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("link", "file_path", "expected"), | ||
| [ | ||
| ("foo.hpp", "src/foo.cpp", "src/foo.hpp"), | ||
| ("../c.h", "src/util/helper.h", "src/c.h"), | ||
| ("/src/foo.hpp", "docs/readme.md", "src/foo.hpp"), | ||
| ("readme.md", "README.md", "readme.md"), | ||
| ("foo.hpp#section", "src/foo.cpp", "src/foo.hpp"), | ||
| ("foo.hpp?v=1", "src/foo.cpp", "src/foo.hpp"), | ||
| ("foo.hpp.", "src/foo.cpp", "src/foo.hpp"), | ||
| ], | ||
| ids=[ | ||
| "sibling file", | ||
| "parent traversal", | ||
| "leading slash is repo root", | ||
| "file at repo root", | ||
| "anchor stripped", | ||
| "query stripped", | ||
| "trailing punctuation stripped", | ||
| ], | ||
| ) | ||
| def test_resolve_link_target(link: str, file_path: str, expected: str) -> None: | ||
| assert resolve_link_target(link, file_path) == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("link", ["https://example.com/x", "http://example.com", "mailto:a@b.com", "#anchor", ""]) | ||
| def test_resolve_link_target_skips_non_relative_links(link: str) -> None: | ||
| assert resolve_link_target(link, "src/foo.cpp") is None | ||
|
|
||
|
|
||
| # --- offset_to_line_and_column -------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_offset_to_line_and_column() -> None: | ||
| content = b"line1\nline2\nXhere" | ||
| assert offset_to_line_and_column(content, 0) == (1, 1) | ||
| assert offset_to_line_and_column(content, 12) == (3, 1) # 'X' after two newlines | ||
|
|
||
|
|
||
| # --- build_set_of_valid_link_targets -------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_build_set_of_valid_link_targets_includes_files_dirs_and_root() -> None: | ||
| assert build_set_of_valid_link_targets(["src/util/helper.h", "README.md"]) == { | ||
| "src/util/helper.h", | ||
| "src/util", | ||
| "src", | ||
| "README.md", | ||
| ".", | ||
| } | ||
|
|
||
|
|
||
| # --- find_broken_links_in_content ----------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_broken_link_is_reported_with_location() -> None: | ||
| assert scan("// see @repo missing/file.md here") == [BrokenLink("src/foo.cpp", 1, 14, "missing/file.md")] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("text", "tracked"), | ||
| [ | ||
| pytest.param("// see @repo foo.hpp for details", ("src/foo.hpp",), id="valid link"), | ||
| pytest.param(" * @report_new: populate the report", (), id="doxygen @report tag"), | ||
| pytest.param('import { Button } from "@repo/ui";', (), id="npm @repo scope"), | ||
| pytest.param("foo@repo missing.md", (), id="marker preceded by word char"), | ||
| pytest.param("// @repo https://example.com/page", (), id="url target"), | ||
| ], | ||
| ) | ||
| def test_scan_reports_no_broken_links(text: str, tracked: tuple[str, ...]) -> None: | ||
| assert scan(text, tracked=tracked) == [] | ||
|
|
||
|
Comment on lines
+89
to
+105
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to pytest.parametrize the relatively trivial cases? We can then use https://docs.pytest.org/en/stable/example/parametrize.html#different-options-for-test-ids to assign names/explanations to those tests |
||
|
|
||
| def test_leading_slash_resolves_from_repo_root() -> None: | ||
| assert scan("// @repo /src/foo.hpp", file_path="docs/readme.md", tracked=("src/foo.hpp",)) == [] | ||
| assert [b.link for b in scan("// @repo /src/nope.hpp", file_path="docs/readme.md")] == ["/src/nope.hpp"] | ||
|
|
||
|
|
||
| def test_multiple_findings_with_correct_lines_and_columns() -> None: | ||
| broken = scan( | ||
| "// @repo foo.hpp\n// @repo missing1.md\ncode();\n/* @repo missing2.md */\n", tracked=("src/foo.hpp",) | ||
| ) | ||
| assert [(b.line, b.column, b.link) for b in broken] == [(2, 10, "missing1.md"), (4, 10, "missing2.md")] | ||
|
|
||
|
|
||
| # --- main ----------------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_main_reports_only_broken_links_across_files( | ||
| fs: FakeFilesystem, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| capsys: pytest.CaptureFixture, | ||
| ) -> None: | ||
| fs.create_file(Path("src/a.cpp"), contents="// @repo missing.h\n") | ||
| fs.create_file(Path("src/b.cpp"), contents="// @repo a.cpp\n") # valid link to the tracked src/a.cpp | ||
| monkeypatch.setattr("dev_tools.check_repo_links.get_repository_root", Path.cwd) | ||
| monkeypatch.setattr("dev_tools.check_repo_links.list_tracked_files", lambda _root: ["src/a.cpp", "src/b.cpp"]) | ||
| assert main(["src/a.cpp", "src/b.cpp"]) == 1 | ||
| assert capsys.readouterr().out == "Found broken links:\nsrc/a.cpp:1:10 missing.h\n" | ||
|
|
||
|
|
||
| def test_main_returns_zero_and_is_silent_for_valid_links( | ||
| fs: FakeFilesystem, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| capsys: pytest.CaptureFixture, | ||
| ) -> None: | ||
| fs.create_file(Path("src/a.cpp"), contents="// @repo b.h\n") | ||
| fs.create_file(Path("src/b.h"), contents="") | ||
| monkeypatch.setattr("dev_tools.check_repo_links.get_repository_root", Path.cwd) | ||
| monkeypatch.setattr("dev_tools.check_repo_links.list_tracked_files", lambda _root: ["src/a.cpp", "src/b.h"]) | ||
| assert main(["src/a.cpp"]) == 0 | ||
| assert capsys.readouterr().out == "" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that I created this, is it still Luminar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving the header as-is for consistency with the rest of the repo, per your note.