Skip to content

docs(tests): stop calling pull requests issues in the const suites - #999

Merged
JarryShaw merged 1 commit into
mainfrom
docs/719-tests-const-pr-citations
Oct 3, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/719-tests-const-pr-citations

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Part of #719.


Description of your pull request and other information

16 sentences under tests/const/ described a pull request as a "GitHub issue". tests/ is exempt from the rule preferring the issue over the pull request (conventions/documentation.rst:203) — but no exemption makes calling a pull request an issue true, and that is the whole of this change.

All 18 changed lines are in-place replacements. Nothing is reflowed, so the marker-merge and inline-literal-split hazards are structurally absent rather than merely checked for; max width, the count of lines over 95 and the reStructuredText marker count are byte-identical per file.

Two of the sixteen were false twice over, not merely mislabelled.

test_const_enum_builtin_parity.py:1138 said #803 added a test. It did not. git log -S on that def finds it first in fc32d1b81, whose subject is PR #677's squash merge; #803's own commit carries the line only as diff context and adds two different tests. Relabelled to PR #677.

Six sites credited "#775/#847's ruling" with converting or sparing mint sites. #841 is the issue #847 landed against, not #775, and it converted nothing — it reordered Socket._missing_ range branches. So PR #847 converted… would restate the same false claim in correct grammar, and #775/#841 fails differently, because the criterion is not in #841's text either: conventions/mint-criterion.rst:52-56 records it as settled in review of #841's branch-order fix and reaffirmed on #775, which puts it in #847's review thread — exactly the case the tests/ exemption exists for. The false noun is dropped and the bare pair kept, matching six siblings already written that way in this directory and test_const_enum_no_mint.py:28's "settled on PR #847 and confirmed on #775".

Eight further candidates were left alone because they already label the pull request correctly — "GitHub issue #862, fixed by #865", "blocked on #859", "the PR #836 ruling" and similar. Separating them needs a scan keyed on the citation run the phrase actually governs, not on any number appearing nearby; the looser method reports 24 sites where 16 are real.

No test pin needed changing — no assertion anywhere in tests/ quotes a string this touches. The seven changed files pass: 244 passed, 40001 subtests, exit code 0 read from the process.

One site outside this tranche, for whoever takes it: tests/foundation/registry/test_protocols.py:535 calls PR #815 an issue.

Part of #719. 16 sentences under tests/const/ described a pull request as a
"GitHub issue". tests/ is exempt from the rule preferring the issue over the
pull request (conventions/documentation.rst:203), but no exemption makes
calling a pull request an issue true.

All 18 changed lines are in-place replacements; nothing is reflowed, so no
marker or inline literal could move. Max width, the count of lines over 95 and
the reStructuredText marker count are byte-identical per file.

Two of the sixteen were false twice over, not merely mislabelled.

- builtin_parity:1138 said #803 added a test. It did not: git log -S finds the
  def first appearing in fc32d1b, whose subject is PR #677's squash merge.
  #803's own commit carries that def only as diff context and adds two other
  tests. Relabelled to PR #677.
- Six sites credited "#775/#847's ruling" with converting or not touching mint
  sites. #847 closes #841, not #775, and it converted nothing -- it reordered
  Socket._missing_ range branches. Neither "PR #847 converted" nor "#775/#841"
  would be true, because the criterion is not in #841's text either:
  conventions/mint-criterion.rst:52-56 records it as settled in review of
  #841's branch-order fix and reaffirmed on #775, so it lives in #847's review
  thread. The false noun is dropped and the bare pair kept, matching six
  siblings already written that way in this directory and no_mint:28's
  "settled on PR #847 and confirmed on #775".

Eight further candidates were left alone because they already label the pull
request correctly -- "fixed by #865", "blocked on #859", "the PR #836 ruling"
and similar. A scan keyed on the citation run the phrase actually governs,
rather than any number nearby, is what separates them.

No test pin needed changing: no assertion anywhere in tests/ quotes a string
this touches.

The seven changed files pass: 244 passed, 40001 subtests, exit code 0 read from
the process.
@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one test Pull requests that add or correct tests (test: subject prefix) labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 7a365d12a — sonnet cross-review, a different model from the author. One real defect, and it was in this description rather than the diff; fixed above, with no new head needed.

The description said "#847 closes #841" and GitHub parsed that as a closing keyword. This pull request was listed under closingIssuesReferences: [841], and #841's own closedBy showed both #847 and #999 — so my prose credited this change with closing an issue it has nothing to do with. #841 was already closed, so nothing broke; the provenance was simply wrong. Reworded to "the pull request that fixed #841", and the reference is gone.

The contestable call holds. I asked the review to attack the #847 decision hardest, and it independently confirmed each limb: #847 closes #841 and reordered Socket._missing_ range branches only, so PR #847 converted… would be false; and the criterion was settled in #847's own comment thread, which it located at 13:06Z, consistent with mint-criterion.rst:52-56 recording it as settled in review of #841's fix. It also found the precedent stronger than I claimed — nine or more bare #775/#847 sites already exist, not six. Its verdict on the outcome: keep the bare pair.

The count that matters is confirmed by a different method. It typed all distinct numbers cited in tests/const/ in one GraphQL call and scanned every issue(s) #N[/#M] run line-wrap-safely: 16 "issue"-labelled references to a pull request at base 3c59e2917, and 0 at head — across 783 ×1, 803 ×2, 815 ×1, 847 ×6, 865 ×2, 907 ×2, 913 ×2. It could not reproduce two of my figures: it counts 61 distinct numbers where I said 49, and did not rebuild the looser 24-site scan. Those are unreconciled and I am not claiming otherwise; the before-and-after figure, which is the one the change rests on, agrees.

It also checked the #677 relabel at the level that matters — not just that #677 is the right number, but that the claim is true of it: fc32d1b81 is #677's squash merge, and it introduces BESPOKE_TEMPLATES along with the tcp-flags render test. One pre-existing imprecision it flagged and I am leaving: "the other half" is loose, since the test covers one module rather than two.

Four of the eight left-alone candidates spot-checked, all genuinely already correct. No collateral damage: 18 insertions and 18 deletions, identical line counts, max widths, over-95 counts and markup-character counts per file, the pre-existing role split at test_const_enum_get.py:98-99 unchanged on both sides, and no assertion in tests/ quoting an edited string. Tests re-run: 244 passed, 40001 subtests, exit 0 read from the process.

Label is now review: good-to-go. Not yet ready to merge — CI is still in flight.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Correction to the comment above: my first fix did not work, and I said it had. I wrote that the stray closing reference was gone. It was not — it survived, because I replaced "closes #841" with "fixed #841", and fix/fixes/fixed is a closing keyword in its own right. I had swapped one trigger for another and then asserted the result without re-reading it.

The sentence now avoids the construction entirely — "#841 is the issue #847 landed against" — and a scan of the whole description for close[sd]?|fix(e[sd])?|resolve[sd]? immediately before a number returns nothing. Verified after the edit rather than assumed: this pull request's closingIssuesReferences is empty, and #841's closedByPullRequestsReferences is back to #847 alone where it briefly read "847, 999".

Worth recording because the trap is broader than the one word: GitHub's closing keywords are a set, not a word, so removing one and leaving another in the same sentence changes nothing, and the only way to know is to re-query the reference after editing. Nothing about the diff changed; this was description prose throughout.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Ready to merge at 7a365d12a — closing the "not yet" on my verdict comment. CI finished clean: 69 success, 3 skipped, 0 in flight, 0 failures, mergeStateStatus is CLEAN, and all six required contexts are green — Required checks passed with Compat Python 3.10 through 3.14, and Compat Python 3.15 (scheduled) skipped.

One commit on main at 3c59e2917. Part of #719; the issue stays open for the rest of its sweep.

Unpublished decisions are yours, and I have not merged. This and #998 are both ready and independent of each other — #998 touches docs/source/conf.py and the conventions pages, this one only tests/const/ — so neither needs the other first. Note that whichever merges first will flip the other to BEHIND, because main's ruleset has strict required checks; that costs the second one a re-run but nothing else.

Still queued behind #998 specifically: the pcapkit/corekit/ citation tranche, 100 conversions already verified, which cannot become a pull request until the roles in #998's conf.py are on main — a converted docstring built against main today errors on an unknown role. And #989 carries a needs: decision on whether the 19 plain # comment citations in that tranche convert at all, which decides 243 sites tree-wide.

@JarryShaw
JarryShaw merged commit 870bef5 into main Oct 3, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the docs/719-tests-const-pr-citations branch October 3, 2026 02:53
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant