diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..cec328be2 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,31 @@ +## What this changes + + + +Fixes # + +## Verification + + + +- [ ] **Ground truth**: at least one assertion checks a specific value against a real SEC filing I verified by hand. Filing and accession: +- [ ] **Regression test** for a bug is in `tests/issues/regression/`, and its docstring links the issue or bead +- [ ] **Cassettes** were recorded from live SEC and not edited afterwards (see CONTRIBUTING.md). Recorded on: +- [ ] **Silence check**: bad or missing input produces a useful error, not `None` +- [ ] **Docs**: any documented example this touches still runs +- [ ] **Changelog**: a `changelog.d/.
.md` fragment, not an edit to `CHANGELOG.md` + +## 6.0 + +- [ ] This PR adds a deprecation, a 6.0 warning, or a "removed in 6.0" note, and `docs/upgrade/6.0.md` says what users see now and what to do instead + +CI fails a PR that stages a 6.0 change without touching the upgrade guide. If it flags something that isn't a 6.0 change, add the `no-upgrade-guide` label, say why here, and re-run the job. + +## Working context (optional) + + diff --git a/.github/workflows/python-hatch-workflow.yml b/.github/workflows/python-hatch-workflow.yml index 1ef03da06..f8564e4cd 100644 --- a/.github/workflows/python-hatch-workflow.yml +++ b/.github/workflows/python-hatch-workflow.yml @@ -150,6 +150,26 @@ jobs: python scripts/check_offline_audit.py fi + # A pull request that stages a 6.0 change (a deprecation, a will-raise + # warning, "removed in 6.0") has to touch docs/upgrade/6.0.md in the same PR. + # #1200/#1201 shipped 5.55.0 deprecations with no guide entry, and nothing + # mechanical noticed (bead edgartools-07lk.14). Pull requests only: the check + # reads the PR's diff, and main has no PR to read. Labels are read live + # rather than from the event payload, so after adding `no-upgrade-guide`, + # re-run this job instead of pushing an empty commit. + - name: Check a staged 6.0 change updates the upgrade guide + if: github.event_name == 'pull_request' + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + gh api --paginate \ + "repos/${{ github.repository }}/pulls/${{ github.event.number }}/files" \ + --jq '.[] | {filename, status, patch} | tojson' > "$RUNNER_TEMP/pr_files.jsonl" + PR_LABELS=$(gh api "repos/${{ github.repository }}/issues/${{ github.event.number }}/labels" \ + --jq '[.[].name] | join(",")') \ + python scripts/check_upgrade_guide.py "$RUNNER_TEMP/pr_files.jsonl" + - name: Run fast tests run: | # Parallelize fast tests - safe, no SEC API calls. diff --git a/changelog.d/07lk-14-upgrade-guide-gate.added.md b/changelog.d/07lk-14-upgrade-guide-gate.added.md new file mode 100644 index 000000000..5d2722618 --- /dev/null +++ b/changelog.d/07lk-14-upgrade-guide-gate.added.md @@ -0,0 +1 @@ +**A gate that fails a pull request which stages a 6.0 change without updating the upgrade guide.** Deprecations, will-raise warnings and "removed in 6.0" notes must now touch `docs/upgrade/6.0.md` in the same PR; replayed over the 265 commits since the guide existed, it flags the four PRs that shipped staging with no guide entry, including #1201. Adds a PR template with the Definition of Done checklist. diff --git a/scripts/check_upgrade_guide.py b/scripts/check_upgrade_guide.py new file mode 100644 index 000000000..9f72dca39 --- /dev/null +++ b/scripts/check_upgrade_guide.py @@ -0,0 +1,201 @@ +#!/usr/bin/env python +"""Fail a pull request that stages a 6.0 change without telling the upgrade guide. + +Every user-visible 6.0 break ships in two halves: a warning in 5.x, then the +break itself. The warning half is only useful if ``docs/upgrade/6.0.md`` says +what to do about it, because that page is where a user porting to 6.0 (or +their AI assistant) looks. A CHANGELOG entry gets written by habit; an +upgrade-guide entry does not, and nothing caught the difference: PRs #1200 and +#1201 shipped the edgar.files deprecations in 5.55.0 with no guide entry, and +the entries had to be reconstructed after release (bead edgartools-07lk.14). + +THE RULE: a pull request whose added lines stage a 6.0 change must also touch +``docs/upgrade/6.0.md``. Staging means any of + +- a call that warns about the future: ``warn_will_raise(...)``, or any function + whose name says ``deprecat`` (``deprecated_alias``, ``_deprecation_warning``) + or is one of the module-local helpers listed in ``WARNING_HELPERS``; +- a ``DeprecationWarning`` or ``FutureWarning`` written into library code; +- the phrase "removed in 6.0" (also "removed in v6.0", "removed in edgartools + 6.0", and the same with "deprecated"); +- a new ``changelog.d/*.deprecated.md`` or ``*.removed.md`` fragment. + +Only ADDED lines in ``edgar/`` count and comment lines are ignored. Markers +are also netted across the PR: each staging marker a PR removes (a +``FutureWarning``, a ``warn_will_raise(`` call) cancels one the PR adds. So +deleting a deprecated name in the 6.0 window, or restructuring or moving a +warning that already exists, does not trip the check; adding a new one does. +MEASURED against history: replaying the 265 commits merged to main since the +guide was created (2026-08-05), 10 staged a change and updated the guide +(pass), and 5 fail. Four of those staged a 6.0 change with no guide entry in the +same PR: #1034, #1201, 2464a531 (07lk.12.1 module moves), and 4e31aed6 +(07lk.11.5 FilingHomepage soup deprecation, which the guide still lacks). The +fifth, #1384, added a new warning site for a change #1379 had just documented, +which is what the label is for. Netting is what cleared #1037, which only +restructured an existing ``warn_will_raise`` call. + +WHERE THE DIFF COMES FROM. The CI checkout is shallow, so there is no merge base +to ``git diff`` against. The workflow asks the GitHub API for the PR's files +instead and pipes them here as JSON lines (one ``{"filename", "status", +"patch"}`` object per line). GitHub omits ``patch`` for very large diffs, and a +file without one is reported rather than silently passed. + +THE ESCAPE HATCH is the ``no-upgrade-guide`` label, for the rare PR that +matches without staging anything (say, a test helper that asserts on +``FutureWarning`` inside ``edgar/``). CI passes the PR's labels in +``PR_LABELS``. Using the label is a claim the reviewer can see, which is the +point: skipping the guide has to be a visible decision. +""" +from __future__ import annotations + +import json +import os +import re +import sys +from collections import Counter +from dataclasses import dataclass +from typing import Iterable, Iterator + +GUIDE = "docs/upgrade/6.0.md" +SKIP_LABEL = "no-upgrade-guide" + +# Module-local helpers that exist only to emit a deprecation warning, whose +# names the generic pattern below does not catch. #1201 staged the whole +# edgar.files removal through warn_legacy_html_usage, which is how this +# check would otherwise have missed the very PR it was written for. +WARNING_HELPERS = ("warn_legacy_html_usage",) + +STAGING = re.compile( + r"\bwarn_will_raise\s*\(" + r"|\b\w*deprecat\w*\s*\(" + r"|\b(?:" + "|".join(WARNING_HELPERS) + r")\s*\(" + r"|\b(?:DeprecationWarning|FutureWarning)\b" + r"|(?:removed|deprecated)\s+in\s+(?:edgartools\s+)?v?6\.0", + re.IGNORECASE, +) + +# A definition is the mechanism, not a use of it. +DEFINITION = re.compile(r"^\s*(?:async\s+)?def\s") + +FRAGMENT = re.compile(r"^changelog\.d/[^/]+\.(?:deprecated|removed)\.md$") + + +@dataclass(frozen=True) +class Hit: + filename: str + line: str + + def __str__(self) -> str: + return f"{self.filename}: {self.line.strip()[:110]}" + + +def _marker(match: re.Match) -> str: + """One spelling per marker, so 'removed in v6.0' nets against 'Removed in 6.0'.""" + return re.sub(r"\s+|edgartools|v(?=6)|\($", "", match.group(0).lower()) + + +def _code_lines(patch: str, sign: str) -> Iterator[str]: + """The added (``+``) or removed (``-``) lines of a patch that are not comments or defs.""" + for raw in patch.splitlines(): + if not raw.startswith(sign) or raw.startswith(sign * 3): + continue + line = raw[1:] + if line.strip().startswith("#") or DEFINITION.match(line): + continue + yield line + + +def removed_markers(patch: str) -> Counter: + """How many of each staging marker a patch removes.""" + return Counter(_marker(m) for line in _code_lines(patch, "-") for m in STAGING.finditer(line)) + + +def staged_lines(filename: str, patch: str, removed: Counter | None = None) -> list[Hit]: + """The added lines of one file's patch that stage a new 6.0 change. + + ``removed`` is the PR-wide count from `removed_markers`. It is consumed as + added markers net against it, so pass one Counter through every file. + """ + removed = Counter() if removed is None else removed + hits = [] + for line in _code_lines(patch, "+"): + markers = [_marker(m) for m in STAGING.finditer(line)] + if not markers: + continue + if all(removed[k] > 0 for k in markers): + removed.subtract(markers) + continue + hits.append(Hit(filename, line)) + return hits + + +@dataclass +class Verdict: + hits: list[Hit] + unreadable: list[str] + guide_touched: bool + + @property + def ok(self) -> bool: + return self.guide_touched or not (self.hits or self.unreadable) + + +def evaluate(files: Iterable[dict]) -> Verdict: + files = list(files) + removed = sum((removed_markers(f["patch"]) for f in files + if f.get("patch") and f["filename"].startswith("edgar/")), Counter()) + hits: list[Hit] = [] + unreadable: list[str] = [] + guide_touched = False + for f in files: + name = f["filename"] + status = f.get("status", "modified") + if name == GUIDE and status != "removed": + guide_touched = True + continue + if FRAGMENT.match(name) and status == "added": + hits.append(Hit(name, "new changelog fragment")) + continue + if not (name.startswith("edgar/") and name.endswith(".py")) or status == "removed": + continue + patch = f.get("patch") + if patch is None: + unreadable.append(name) + continue + hits.extend(staged_lines(name, patch, removed)) + return Verdict(hits, unreadable, guide_touched) + + +def main(argv: list[str]) -> int: + labels = {s.strip() for s in os.environ.get("PR_LABELS", "").split(",") if s.strip()} + source = open(argv[1], encoding="utf-8") if len(argv) > 1 else sys.stdin + with source: + files = [json.loads(line) for line in source if line.strip()] + + verdict = evaluate(files) + if verdict.ok: + reason = "the upgrade guide is updated" if verdict.hits else "nothing stages a 6.0 change" + print(f"OK: {reason} ({len(files)} files checked).") + return 0 + if SKIP_LABEL in labels: + print(f"SKIPPED by the '{SKIP_LABEL}' label; {len(verdict.hits)} staging line(s) not documented:") + for hit in verdict.hits: + print(f" {hit}") + return 0 + + print(f"This PR stages a 6.0 change but does not touch {GUIDE}.\n") + for hit in verdict.hits: + print(f" {hit}") + for name in verdict.unreadable: + print(f" {name}: diff too large for the API to return; could not be checked") + print( + f"\nAdd what the user sees in 5.x and what they should do instead to {GUIDE}" + " (the 'What 6.0 changes' table for None -> raise changes, its own section for a" + f" move or removal). If nothing here stages a 6.0 change, add the '{SKIP_LABEL}'" + " label and say why in the PR description." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/tests/meta/test_upgrade_guide_gate.py b/tests/meta/test_upgrade_guide_gate.py new file mode 100644 index 000000000..c6e885bbe --- /dev/null +++ b/tests/meta/test_upgrade_guide_gate.py @@ -0,0 +1,156 @@ +"""The upgrade-guide gate has to catch the PR it was written for, and stay quiet otherwise. + +``scripts/check_upgrade_guide.py`` fails a pull request that stages a 6.0 +change without touching ``docs/upgrade/6.0.md``. It exists because #1200/#1201 +shipped the edgar.files deprecations in 5.55.0 with no guide entry, and that PR +warned through a module-local helper and the phrase "removed in edgartools 6.0" +rather than a bare ``DeprecationWarning`` — so the first test is that shape, +verbatim. + +The patches here are the shape the GitHub pull-request files API returns, which +is what the workflow pipes into the script. + +Bead: edgartools-07lk.14 +""" + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +# Pure functions over strings, plus one call to main() with a temp file. +pytestmark = pytest.mark.fast + +ROOT = Path(__file__).resolve().parents[2] +SCRIPT = ROOT / "scripts" / "check_upgrade_guide.py" + + +def _load(): + """Import the script by path — `scripts/` is not a package.""" + spec = importlib.util.spec_from_file_location("check_upgrade_guide", SCRIPT) + module = importlib.util.module_from_spec(spec) + sys.modules["check_upgrade_guide"] = module + spec.loader.exec_module(module) + return module + + +gate = _load() + +GUIDE = {"filename": "docs/upgrade/6.0.md", "status": "modified", "patch": "@@ -1 +1 @@\n+a line\n"} + +# From #1201 (0878e3ce), edgar/files/markdown.py, trimmed to the lines that matter. +PR_1201_MARKDOWN = ( + "@@ -1,0 +2,9 @@\n" + "+from edgar.files._deprecation import warn_legacy_html_usage\n" + "+_MARKDOWN_DEPRECATION_MSG = (\n" + '+ "removed in edgartools 6.0. Use Filing.markdown() / Attachment.markdown(), "\n' + "+)\n" + "+ warn_legacy_html_usage(_MARKDOWN_DEPRECATION_MSG)\n" +) + + +def _py(patch, name="edgar/files/markdown.py", status="modified"): + return {"filename": name, "status": status, "patch": patch} + + +def test_the_pr_that_motivated_the_gate_fails_it(): + verdict = gate.evaluate([_py(PR_1201_MARKDOWN)]) + assert not verdict.ok + # The message line and the helper call; the import line is not staging. + assert [h.line.strip() for h in verdict.hits] == [ + '"removed in edgartools 6.0. Use Filing.markdown() / Attachment.markdown(), "', + "warn_legacy_html_usage(_MARKDOWN_DEPRECATION_MSG)", + ] + + +def test_touching_the_guide_passes_the_same_pr(): + assert gate.evaluate([_py(PR_1201_MARKDOWN), GUIDE]).ok + + +@pytest.mark.parametrize("line", [ + "warn_will_raise(section_not_found(report, item), stacklevel=4)", + "warnings.warn(msg, DeprecationWarning, stacklevel=2)", + " FutureWarning,", + "__getattr__ = deprecated_alias(InvalidDateException=InvalidDateError)", + '"FilingHomepage(soup=...) is deprecated and will be removed in v6.0. "', + '"""Deprecated: use edgar.exceptions.DataObjectError. Removed in 6.0.', +]) +def test_each_staging_idiom_in_the_codebase_is_caught(line): + assert gate.staged_lines("edgar/x.py", f"@@ -0,0 +1 @@\n+{line}\n") + + +@pytest.mark.parametrize("line", [ + "# Deliberately not warn_will_raise(): the parser's own error propagates", + "def warn_will_raise(error, *, stacklevel=3):", + "return parse_homepage_html(str(root))", + "# removed in 6.0", +]) +def test_comments_definitions_and_ordinary_code_are_not_staging(line): + assert gate.staged_lines("edgar/x.py", f"@@ -0,0 +1 @@\n+{line}\n") == [] + + +def test_removed_lines_never_count(): + patch = "@@ -1,2 +0,0 @@\n-warnings.warn(msg, DeprecationWarning)\n-# removed in 6.0\n" + assert gate.evaluate([_py(patch)]).ok + + +def test_restructuring_an_existing_warning_nets_to_nothing(): + # #1037's shape: a multi-line warn_will_raise( call became a one-liner. + patch = ( + "@@ -1,3 +1,2 @@\n" + "- warn_will_raise(\n" + "- ValidationError(msg))\n" + "+ warn_will_raise(malformed)\n" + ) + assert gate.evaluate([_py(patch, name="edgar/__init__.py")]).ok + + +def test_netting_works_across_files_and_does_not_over_forgive(): + moved_out = _py("@@ -1 +0,0 @@\n- warnings.warn(m, FutureWarning)\n", name="edgar/a.py") + moved_in_twice = _py( + "@@ -0,0 +1,2 @@\n+ warnings.warn(m, FutureWarning)\n+ warnings.warn(n, FutureWarning)\n", + name="edgar/b.py", + ) + verdict = gate.evaluate([moved_out, moved_in_twice]) + assert len(verdict.hits) == 1 # one moved, one new + + +def test_a_deprecated_or_removed_changelog_fragment_is_staging(): + added = {"filename": "changelog.d/1326.deprecated.md", "status": "added"} + assert not gate.evaluate([added]).ok + assert gate.evaluate([{"filename": "changelog.d/1379.fixed.md", "status": "added"}]).ok + + +def test_files_outside_the_library_are_ignored(): + test_file = _py("@@ -0,0 +1 @@\n+ with pytest.warns(FutureWarning):\n", name="tests/test_x.py") + assert gate.evaluate([test_file]).ok + + +def test_a_patch_the_api_omitted_is_reported_not_passed(): + verdict = gate.evaluate([{"filename": "edgar/_filings.py", "status": "modified", "patch": None}]) + assert not verdict.ok + assert verdict.unreadable == ["edgar/_filings.py"] + + +def _run(tmp_path, files, monkeypatch, labels=""): + path = tmp_path / "files.jsonl" + path.write_text("\n".join(json.dumps(f) for f in files)) + monkeypatch.setenv("PR_LABELS", labels) + return gate.main(["check_upgrade_guide.py", str(path)]) + + +def test_main_fails_names_the_lines_and_says_what_to_do(tmp_path, monkeypatch, capsys): + assert _run(tmp_path, [_py(PR_1201_MARKDOWN)], monkeypatch) == 1 + out = capsys.readouterr().out + assert "does not touch docs/upgrade/6.0.md" in out + assert "edgar/files/markdown.py: warn_legacy_html_usage(_MARKDOWN_DEPRECATION_MSG)" in out + assert "no-upgrade-guide" in out + + +def test_the_label_passes_but_still_lists_what_it_skipped(tmp_path, monkeypatch, capsys): + assert _run(tmp_path, [_py(PR_1201_MARKDOWN)], monkeypatch, labels="bug,no-upgrade-guide") == 0 + out = capsys.readouterr().out + assert out.startswith("SKIPPED") + assert "warn_legacy_html_usage" in out