diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 026a78c..27fea0f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,7 +75,7 @@ python3 scripts/generate_og_images.py --force # re-render every existing OG ima make fix-explainer-count # correct any stale "N explainers" mention in README/CONTRIBUTORS/METRICS/ROADMAP after adding/removing an explainer make key-fold # regenerate the JS case-folding table (assets/profiler-engine.js KEY_FOLD) after a Unicode upgrade make favicons # regenerate favicon.ico, apple-touch-icon.png, icon-{192,512}.png after editing logo.svg -make external-links # report dead external links (a report, never a gate); a link that is correct but cannot pass an anonymous check goes in scripts/external-links-ignore.txt as `URL reason`, and the report still counts it +make external-links # report dead external links (a report, never a gate); a link that is correct but cannot pass an anonymous check goes in scripts/external-links-ignore.txt as `URL reason`, and the report still counts it; a link that moved permanently (301 or 308) is listed with its new address make lint # em-dash-free check + broken internal doc link check + issue-number check + explainer counts + traction figures + dataset source links + CONTRIBUTORS.md vs merged PRs + ruff ``` diff --git a/scripts/check_external_links.py b/scripts/check_external_links.py index 92a961b..4ffb2cf 100644 --- a/scripts/check_external_links.py +++ b/scripts/check_external_links.py @@ -17,6 +17,8 @@ the DOI handle API is used to distinguish a registered identifier from a missing one. Other 404, 410 and a name that does not resolve are broken. Everything else (403, 429, 5xx, timeouts, refused connections) is unknown: it is listed but never counted as broken, because it usually means the site blocks scripts or is down. +A link whose redirect chain starts with a permanent 301 or 308 and ends on a page that answers is moved: the +report lists it with its new address so the document can be updated, and it is never counted as broken (#1196). A link that is correct but cannot pass an anonymous check (a page behind a login, a deleted account a document names on purpose) goes in scripts/external-links-ignore.txt: one URL per line, then a reason. The URL is not @@ -46,6 +48,7 @@ BARE = re.compile(r"<(https?://[^>\s]+)>") SKIP_HOSTS = {"localhost", "127.0.0.1", "example.com", "www.example.com"} BROKEN_STATUSES = {404, 410} +MOVED_STATUSES = {301, 308} IGNORE_FILE = ROOT / "scripts" / "external-links-ignore.txt" @@ -99,7 +102,10 @@ def collect_urls(paths, skip_hosts=SKIP_HOSTS) -> dict[str, list[str]]: def check_url(url: str, timeout: float = 15.0) -> tuple[str, str]: - """(status class, detail) with the class one of 'ok', 'broken' or 'unknown'.""" + """(status class, detail) with the class one of 'ok', 'moved', 'broken' or 'unknown'. + + For 'moved' the detail is the address the redirect chain ended on. + """ for method in ("HEAD", "GET"): try: try: @@ -122,7 +128,27 @@ def check_url(url: str, timeout: float = 15.0) -> tuple[str, str]: def _open(url: str, method: str, user_agent: str, timeout: float) -> tuple[str, str]: request = urllib.request.Request(url, method=method, headers={"User-Agent": user_agent, "Accept": "*/*"}) with urllib.request.urlopen(request, timeout=timeout, context=_ssl_context()) as response: - return "ok", str(response.status) + final_url, status = response.url, str(response.status) + if final_url != url and _first_redirect_status(request, timeout) in MOVED_STATUSES: + return "moved", final_url + return "ok", status + + +class _NoRedirect(urllib.request.HTTPRedirectHandler): + def redirect_request(self, req, fp, code, msg, headers, newurl): + return None # urllib then raises HTTPError with the redirect's own status + + +def _first_redirect_status(request: urllib.request.Request, timeout: float) -> int | None: + """The status of the first hop for a URL that redirected, so a temporary 302 or 307 stays 'ok'.""" + opener = urllib.request.build_opener(urllib.request.HTTPSHandler(context=_ssl_context()), _NoRedirect()) + try: + with opener.open(request, timeout=timeout) as response: + return response.status + except urllib.error.HTTPError as error: + return error.code + except (urllib.error.URLError, TimeoutError, OSError): + return None def _classify_http_error(error: urllib.error.HTTPError, url: str, method: str, @@ -183,7 +209,7 @@ def run(urls: dict[str, list[str]], limit: int | None = None, workers: int = 8, stale_ignored = [{"url": url, "reason": reason} for url, reason in sorted(ignore.items()) if url not in urls] urls = {url: files for url, files in urls.items() if url not in ignore} items = sorted(urls)[:limit] if limit else sorted(urls) - results = {"ok": [], "broken": [], "unknown": []} + results = {"ok": [], "moved": [], "broken": [], "unknown": []} with concurrent.futures.ThreadPoolExecutor(workers) as pool: for url, (kind, detail) in zip(items, pool.map(lambda u: check_url(u, timeout), items)): results[kind].append({"url": url, "detail": detail, "files": urls[url]}) @@ -195,12 +221,16 @@ def render(report: dict) -> str: stale_ignored = report.get("stale_ignored", []) lines = [f"Checked {report['checked']} external links: {len(report['ok'])} ok, " f"{len(report['broken'])} broken, {len(report['unknown'])} unknown." + + (f" {len(report['moved'])} moved." if report.get("moved") else "") + (f" {len(report['ignored'])} ignored." if report.get("ignored") else "")] for title, key in (("Broken (404, 410 or a name that does not resolve)", "broken"), ("Unknown (blocked, rate limited or unreachable; not counted as broken)", "unknown")): if report[key]: lines += ["", title + ":"] lines += [f" {item['detail']:>22} {item['url']} ({', '.join(item['files'][:3])})" for item in report[key]] + if report.get("moved"): + lines += ["", "Moved (permanent 301 or 308 redirect; update the link, not counted as broken):"] + lines += [f" {item['url']} -> {item['detail']} ({', '.join(item['files'][:3])})" for item in report["moved"]] if report.get("ignored"): lines += ["", "Ignored (listed in scripts/external-links-ignore.txt, not requested):"] lines += [f" {item['url']} ({item['reason']})" for item in report["ignored"]] diff --git a/tests/test_check_external_links.py b/tests/test_check_external_links.py index 2d20d0f..437ec99 100644 --- a/tests/test_check_external_links.py +++ b/tests/test_check_external_links.py @@ -27,9 +27,13 @@ def _respond(self): "/nohead": 200}.get(self.path, 404) if self.path == "/nohead" and self.command == "HEAD": code = 405 - if self.path == "/redirect": - self.send_response(302) - self.send_header("Location", "/ok") + redirects = {"/redirect": (302, "/ok"), "/temporary": (307, "/ok"), "/moved": (301, "/ok"), + "/moved-permanently": (308, "/ok"), "/moved-twice": (301, "/redirect"), + "/moved-to-gone": (301, "/gone")} + if self.path in redirects: + status, location = redirects[self.path] + self.send_response(status) + self.send_header("Location", location) self.end_headers() return self.send_response(code) @@ -51,7 +55,8 @@ def server(): thread.join() -@pytest.mark.parametrize("path,kind", [("/ok", "ok"), ("/redirect", "ok"), ("/nohead", "ok"), ("/gone", "broken"), +@pytest.mark.parametrize("path,kind", [("/ok", "ok"), ("/redirect", "ok"), ("/temporary", "ok"), ("/nohead", "ok"), + ("/moved", "moved"), ("/moved-permanently", "moved"), ("/gone", "broken"), ("/removed", "broken"), ("/blocked", "unknown"), ("/slow-down", "unknown"), ("/down", "unknown")]) def test_each_status_class(server, path, kind): @@ -157,7 +162,7 @@ def test_main_reports_without_failing_unless_asked(server, tmp_path, capsys): assert script.main([str(doc), "--timeout", "5", "--json", "--limit", "2", "--check-local"]) == 0 report = json.loads(capsys.readouterr().out) assert report["checked"] == 2 and set(report) == { - "checked", "ok", "broken", "unknown", "ignored", "stale_ignored" + "checked", "ok", "moved", "broken", "unknown", "ignored", "stale_ignored" }