-
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 1 commit
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,125 @@ | ||||||
| # 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 contextlib | ||||||
| import posixpath | ||||||
| import re | ||||||
| import subprocess | ||||||
| import sys | ||||||
| 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 | ||||||
| from pathlib import Path | ||||||
|
|
||||||
| # 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("/"): | ||||||
| base, relative = "", link[1:] | ||||||
| else: | ||||||
| base, relative = posixpath.dirname(file_path), link | ||||||
| return posixpath.normpath(posixpath.join(base, relative)) | ||||||
|
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. Consider returning early from this first if, this should simplify logic and then not require an else |
||||||
|
|
||||||
|
|
||||||
| 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, | ||||||
| target_exists: 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: list[BrokenLink] = [] | ||||||
|
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.
Suggested change
|
||||||
| 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 target_exists(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.append(BrokenLink(file_path, line, column, link)) | ||||||
| return broken | ||||||
|
|
||||||
|
|
||||||
| def find_broken_links(files: list[Path], target_exists: Callable[[str], bool]) -> list[BrokenLink]: | ||||||
|
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.
Suggested change
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 rename to |
||||||
| broken: list[BrokenLink] = [] | ||||||
|
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.
Suggested change
|
||||||
| for file in files: | ||||||
| with contextlib.suppress(OSError): | ||||||
|
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. Why do we want this? I.e., in which cases do we want to ignore an error from trying to read a file?
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. Agreed, it was unnecessary defensive code — pre-commit only passes staged, readable files. Removed; files are now read directly. |
||||||
| broken.extend(find_broken_links_in_content(file.read_bytes(), file.as_posix(), target_exists)) | ||||||
| return broken | ||||||
|
|
||||||
|
|
||||||
| def build_valid_target_set(tracked_files: Iterable[str]) -> set[str]: | ||||||
|
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.
Suggested change
|
||||||
| """Build the set of valid link targets: every tracked file plus all of their parent directories.""" | ||||||
| valid = set(tracked_files) | ||||||
|
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.
Suggested change
|
||||||
| for path in list(valid): | ||||||
| parent = posixpath.dirname(path) | ||||||
| while parent: | ||||||
| valid.add(parent) | ||||||
| parent = posixpath.dirname(parent) | ||||||
| valid.add(".") # the repository root itself | ||||||
|
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. This makes the script dependent on where it's invoked from, right? While that's guaranteed with prek/pre-commit, let's not rely on it. What's the best way of doing this instead? Since we already rely on git, should we use that to find the repo root?
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. Good catch. Now uses |
||||||
| return valid | ||||||
|
|
||||||
|
|
||||||
| def list_tracked_files() -> list[str]: | ||||||
| output = subprocess.run(["git", "ls-files", "-z"], capture_output=True, check=True).stdout # noqa: S607 | ||||||
| return [path.decode("utf-8", "surrogateescape") for path in output.split(b"\0") if path] | ||||||
|
|
||||||
|
|
||||||
| def report_broken_links(broken: list[BrokenLink]) -> bool: | ||||||
| if not broken: | ||||||
| return False | ||||||
|
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 separate concerns: make this function a pure printing function and decide the return value in main |
||||||
| print("Found broken links:") | ||||||
| for link in broken: | ||||||
| print(f"{link.file_path}:{link.line}:{link.column} {link.link}") | ||||||
| return True | ||||||
|
|
||||||
|
|
||||||
| def main(argv: Sequence[str] | None = None) -> int: | ||||||
| files = parse_arguments(argv).filenames | ||||||
| target_exists = build_valid_target_set(list_tracked_files()).__contains__ | ||||||
| return 1 if report_broken_links(find_broken_links(files, target_exists)) else 0 | ||||||
|
|
||||||
|
|
||||||
| if __name__ == "__main__": | ||||||
| sys.exit(main()) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| # 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_valid_target_set, | ||
| find_broken_links, | ||
| find_broken_links_in_content, | ||
| main, | ||
| offset_to_line_and_column, | ||
| report_broken_links, | ||
| resolve_link_target, | ||
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
|
|
||
| from pyfakefs.fake_filesystem import FakeFilesystem | ||
|
|
||
| TRACKED = ["README.md", "docs/readme.md", "src/foo.hpp", "src/util/helper.h", "src/c.h"] | ||
|
|
||
|
|
||
| def exists(*paths: str) -> Callable[[str], bool]: | ||
| return build_valid_target_set(paths).__contains__ | ||
|
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 make this (with a better name, probably) a production code function that we also use in main? |
||
|
|
||
|
|
||
| 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, exists(*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"), # sibling file | ||
|
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. Instead of making this a comment, use pytest functionality to give this test case a name. I think you want to use https://docs.pytest.org/en/stable/example/parametrize.html#different-options-for-test-ids |
||
| ("../c.h", "src/util/helper.h", "src/c.h"), # parent traversal | ||
| ("/src/foo.hpp", "docs/readme.md", "src/foo.hpp"), # leading slash = repo root | ||
| ("readme.md", "README.md", "readme.md"), # file at the repo root | ||
| ("foo.hpp#section", "src/foo.cpp", "src/foo.hpp"), # anchor stripped | ||
| ("foo.hpp?v=1", "src/foo.cpp", "src/foo.hpp"), # query stripped | ||
| ("foo.hpp.", "src/foo.cpp", "src/foo.hpp"), # trailing prose 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_valid_target_set ----------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_build_valid_target_set_includes_files_dirs_and_root() -> None: | ||
| valid = build_valid_target_set(["src/util/helper.h", "README.md"]) | ||
| assert valid.issuperset({"src/util/helper.h", "src/util", "src", "README.md", "."}) | ||
| assert "src/missing.h" not in valid | ||
|
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. Why can't we assert set equality? |
||
|
|
||
|
|
||
| # --- 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")] | ||
|
|
||
|
|
||
| def test_valid_link_is_not_reported() -> None: | ||
| assert scan("// see @repo foo.hpp for details", tracked=("src/foo.hpp",)) == [] | ||
|
|
||
|
|
||
| def test_doxygen_param_tag_is_ignored() -> None: | ||
| # `@report...` has no whitespace after `@repo`, so the marker never matches. | ||
| assert scan(" * @report_new: populate the report\n * @reported: bool") == [] | ||
|
|
||
|
|
||
| def test_npm_scope_is_ignored() -> None: | ||
| # `@repo/ui` is followed by `/`, not whitespace, so it is not our marker. | ||
| assert scan('import { Button } from "@repo/ui";') == [] | ||
|
|
||
|
|
||
| def test_marker_must_not_follow_word_character() -> None: | ||
| assert scan("foo@repo missing.md") == [] | ||
|
|
||
|
|
||
| def test_url_target_is_skipped() -> None: | ||
| assert scan("// @repo https://example.com/page") == [] | ||
|
|
||
|
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")] | ||
|
|
||
|
|
||
| def test_prefilter_skips_files_without_marker() -> None: | ||
| assert scan("int main() { return 0; }") == [] | ||
|
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. Why do we need this test? I think tests shouldn't know about existence of such a prefilter. And we have tests that cover cases in which the hook should do nothing. |
||
|
|
||
|
|
||
| # --- find_broken_links (file system) -------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_find_broken_links_reads_files(fs: FakeFilesystem) -> 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") | ||
| broken = find_broken_links([Path("src/a.cpp"), Path("src/b.cpp")], exists("src/a.cpp")) | ||
| assert broken == [BrokenLink("src/a.cpp", 1, 10, "missing.h")] | ||
|
|
||
|
|
||
| def test_find_broken_links_ignores_unreadable_files() -> None: | ||
| assert find_broken_links([Path("does/not/exist.cpp")], exists()) == [] | ||
|
|
||
|
|
||
| # --- report_broken_links -------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_report_broken_links_prints_findings(capsys: pytest.CaptureFixture) -> None: | ||
| assert report_broken_links([BrokenLink("src/a.cpp", 3, 12, "missing.h")]) | ||
| out = capsys.readouterr().out | ||
| assert "Found broken links:" in out | ||
| assert "src/a.cpp:3:12 missing.h" in out | ||
|
|
||
|
|
||
| def test_report_broken_links_is_silent_when_empty(capsys: pytest.CaptureFixture) -> None: | ||
| assert not report_broken_links([]) | ||
| assert not capsys.readouterr().out | ||
|
|
||
|
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 cover these entirely with the |
||
|
|
||
| # --- main ----------------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_main_returns_one_for_broken_links( | ||
| fs: FakeFilesystem, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| capsys: pytest.CaptureFixture, | ||
| ) -> None: | ||
| fs.create_file(Path("src/a.cpp"), contents="// @repo missing.h\n") | ||
| monkeypatch.setattr("dev_tools.check_repo_links.list_tracked_files", lambda: ["src/a.cpp"]) | ||
| assert main(["src/a.cpp"]) == 1 | ||
| assert "src/a.cpp:1:10 missing.h" in capsys.readouterr().out | ||
|
|
||
|
|
||
| def test_main_returns_zero_for_valid_links(fs: FakeFilesystem, monkeypatch: pytest.MonkeyPatch) -> 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.list_tracked_files", lambda: ["src/a.cpp", "src/b.h"]) | ||
| assert main(["src/a.cpp"]) == 0 | ||
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.