Skip to content

docs(protocols): state the rulings behind mh, vendor and exceptions in our own words - #991

Merged
JarryShaw merged 3 commits into
mainfrom
docs/987-protocols-vendor-statements
Oct 2, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
docs/987-protocols-vendor-statements

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • docs — documentation only

Description of your pull request and other information

Refs #987, #719. Group C of the quotation-to-statement conversion: nine quoted rulings in three files become statements with the reason and the issue cited, in the locative form (a ruling given in review of the work for #NNN).

Docstrings only: token streams are identical before and after with string literals masked. tests/protocols/internet/test_mh_unit.py 52 passed / 499 subtests; tests/utilities/ 121 passed.

…n our own words

Refs #987, #719.

- mh.py: four docstring passages quoted the maintainer; restate the
  ruling on #935 (delete both get overrides rather than widen them) and
  what it bought.
- vendor/__main__.py: restate the #872 snapshot-and-revert ruling and the
  keep-zero-exited-targets rule instead of quoting them.
- utilities/exceptions.py: restate the stdlib-shaped exception ruling
  carried out by #923.

Docstrings only; token streams identical with string literals masked.
@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 labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at eb00cd9 — opus cross-review. One blocking provenance error; everything else held under independent measurement.

Blocker: mh.py:584-585 and :706-707 attribute the lean to a review that did not exist yet. Both read that a ruling given in review of the work for #935 first leaned toward the widening and then reversed. I verified the provenance myself:

So the lean predates #940 entirely and is what caused it to be opened as a widening attempt — it cannot have been given in review of it. The text also contradicts the sentence before it, which says the first attempt already widened. This pull request already carries the right form 260 lines later at :853/:932, an earlier lean on the issue; the fix is to use it at both #935 sites. It is the defect class #987 exists to remove, which is why it blocks rather than being a nit.

Non-blocking, going in with it: a dangling modifier at :853/:932 where the trailing clause can attach to either verb and only one is true; two imprecisions at exceptions.py:719-720, where "carried out by #923" sits against the #877 re-parenting that #921 actually carried out as its phase 2; and two re-wrap artefacts at mh.py:593 and :715.

Confirmed otherwise. Prose-only in all three files, and checked harder than by counts alone: masked-token sequences are byte-identical and the only six differing string tokens are each a class's or function's first statement, so no __all__ entry, format string, error message or default argument moved. Both get overrides and their type: ignore[override] suppressions really are gone. Line length unchanged. Tests reproduce exactly: 52 passed / 499 subtests, and 121 passed / 104 subtests.

Two things the pull request under-claimed and should get credit for: the old prose mislabelled the discard instruction as the ruling's option (b), which was a different cron-workflow option, and it merged two rulings 2h19m apart into one.

Also folding in a pre-existing defect in a file this already edits: vendor/__main__.py:105 cross-references Vendor._write_atomic, which exists nowhere in pcapkit/vendor/ on this branch or on main, while the same sentence says it was deleted.

The verdict carries over from 31a0adc: the update-branch merge brought main in without touching any of the three files.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
The cross-review on #991 found mh.py placing the lean toward widening in a
review that did not exist yet. Verified against the API:

- the lean is comment 5901325827 on issue #935, 2026-09-29T23:57:26Z, given on
  the issue itself rather than in any review;
- the reversal is comment 5902590532 on the widening attempt, 2026-09-30
  T02:00:58Z;
- that attempt's earliest comment of any kind is 2026-09-30T01:45:20Z, so the
  lean predates it by 1h47m48s and is what caused it to be opened.

The passage now reads as an earlier lean on the issue, overtaken by a later
ruling in review of the attempt, matching the form this file already used 260
lines further down.

- The trailing clause at the two sibling sites could attach to either verb,
  and only the widening was ever preferred; reworded so it cannot invert.
- exceptions.py named the #877 re-parenting adjacent to the issue that did not
  carry it out. #877's phase 2 is the half the ruling was reviewed on.
- vendor/__main__.py cross-referenced Vendor._write_atomic with :meth:, which
  resolves to nothing: git grep finds the method nowhere under pcapkit/vendor/
  on this branch or on main, and the same sentence says it was deleted. Now a
  plain literal.
- Re-flowed the five touched paragraphs; remaining short lines are forced by a
  following role too long to fit.

Prose only: masked-token sequences identical in all three files, every
differing string token is a docstring, and the AST with docstrings blanked
compares equal. Maximum line length unchanged at 190/99/102. Tests: 52 passed
/ 499 subtests, and 121 passed / 104 subtests.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Fixed and pushed as ba49e0f94 — all four review items plus the pre-existing dangling target.

The fix author independently confirmed the provenance table before writing: the lean is comment 5901325827 on issue #935 at 23:57:26Z, the reversal is 5902590532 on the widening attempt at 02:00:58Z, and that attempt's earliest comment of any kind is 5902430148 at 01:45:20Z. The passage now reads as an earlier lean on the issue overtaken by a later ruling in review of the attempt, matching the form this file already used 260 lines further down.

The mechanical item took two rounds. The first attempt moved the raggedness rather than removing it, leaving widen them, so at 22 characters mid-sentence. It is re-flowed now at 29-76 characters, and I checked every remaining short line is forced by a following role too long to fit — line 722 is followed by a 62-character fully-qualified :class: role, where the sibling passage uses the short form and fits in the same space.

Verified by me rather than relayed: masked-token sequences identical in all three files, every differing string token is a docstring, and the AST with docstrings blanked compares equal. Maximum line length unchanged at 190 / 99 / 102. Tests reproduce: 52 passed / 499 subtests, and 121 passed / 104 subtests.

One residual imprecision I am accepting rather than papering over, with the reasoning raised on #987: "#877's phase-2 re-parenting" does not uniquely identify where the ruling was reviewed, because phase 2 has two halves — #921 covered 17 of the 24 non-registry enumerations and #930 the remaining seven, and #930's half is cited two files away. Pinning it would mean naming the pull request, which the convention forbids.

Label back to review: pending; a fresh cross-review is running against ba49e0f94.

@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 2, 2026
@JarryShaw JarryShaw added the review: pending No verdict for the current head - never reviewed, or the head moved since the last one label Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at ba49e0f94 — opus cross-review, round 2. No blocking finding. It also caught two errors of mine, both confirmed, so those first.

My duration figure was wrong. I said the lean predates #940's earliest comment by 1h47m48s, here and in the commit message. From 2026-09-29T23:57:26Z to 2026-09-30T01:45:20Z is 1:47:54. The conclusion is unaffected — the lean still precedes the pull request it caused — but the number as published was not the number I measured.

My "every remaining short line is token-forced" claim was overbroad. True for mh.py, where all nine changed lines are forced at the 76-column fill. False for two: vendor/__main__.py:114 joins to exactly 76 and exceptions.py:720 to 75, so both could absorb their successor. I verified one line and generalised from it — the same move I blocked #990 for an hour ago. Neither is worth a revision: absorbing -type would split the literal mid-flag, and absorbing whichever would cascade a re-wrap through an untouched paragraph to shrink a two-line diff. The defensible version of what I meant is the reviewer's: at 76 columns the re-flowed lines are better packed than the unchanged prose around them, which still has 34, 2 and 11 absorbable lines respectively.

Independently verified at this head, beyond what I had: all three token counts reproduced exactly (31432 / 1146 / 556), sequences identical, ASTs equal with docstrings blanked, and every differing string token confirmed a docstring by position. The role multiset is unchanged except the intended one-for-one _write_atomic swap, and the split literal was pushed through docutils — zero system messages, and it renders as one space across the break. The measured claims were checked by import rather than from the threads: __members__ and list(cls) really do agree at 6 and at 4, none of the seven classes mints an alias, and all seven converge on EnumValueError. Tests 173 passed / 603 subtests, matching 52+121 and 499+104.

Two residual items, neither blocking and neither in this pull request's file set to fix:

Labelling review: good-to-go. Unpublished and unmerged — yours to merge.

@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 added a commit that referenced this pull request Oct 2, 2026
tests/vendor/test_vendor_snapshot_restore_unit.py:14 and :58 carried
:meth: roles pointing at Vendor._write_atomic. Nothing defines it: grep
finds no `def _write_atomic` anywhere in the tree, and pcapkit/vendor/
default.py has no such method. Line 14 is the odder of the two, since the
sentence it sits in says the method no longer exists while using a role
that asserts it does.

The history is sharper than "removed". `def _write_atomic` appears only in
6fde283 and a0748fb, both intermediate revisions of #873 that the
squash dropped, so the method never reached main at all. Under pcapkit/ the
only commit touching the name is that squash, 6877210, and what it added
was the dangling role in __main__.py rather than the method -- #991 fixes
that one.

- Both roles become plain inline literals, so the prose still names the
  method without claiming a resolvable target.

tests/ sits outside the Sphinx source tree, which is why neither emitted a
build warning. A sweep of all 346 roles across tests/vendor/ found no other
unresolved target.

Prose only: both sites are inside the module docstring, which spans lines
2-165. Token sequences are identical with strings masked, the AST with
docstrings blanked compares equal, and maximum line length stays 99.
JarryShaw added a commit that referenced this pull request Oct 2, 2026
Part of #987. Five quotations across two files become statements, and one
citation is corrected from authority to provenance.

sentinels.py's comment above ABSENT read "per GitHub issue #937" for the fact
that ABSENT is private by documentation rather than by underscore. That is a
what-changed statement, not a ruling attribution: #937 is the SCREAMING_SNAKE
rename, and its own body attributes the ruling to #719, which carries it. Now
written with an action verb naming the issue the change belongs to.

- Module docstring: the one-shared-module choice on #911 stated rather than
  quoted, naming what it was chosen over.
- NoDefaultType: the dedicated-class ruling and the unsettled-naming remark,
  both given in review of the work for #857, stated as what they settled.
- AbsentType: the rename ruling on #719 stated, including that documenting
  ABSENT as private replaces the underscore.
- tests/corekit: two "I prefer (2) directly" quotations become statements. The
  pull-request citations stay, since tests/ is exempt from the issue-citation
  rule per docs/source/contributing/conventions/documentation.rst:203-210, and
  the wording matches what landed for the same ruling in #991.

Three other quoted spans are left alone deliberately: two are documentation
section titles and one is a caveat the docstring makes about the code, none of
them a maintainer's words.

Prose only: token sequences identical with strings masked in both files, the
AST with docstrings blanked compares equal, and maximum line length is
unchanged at 98 and 121.
@JarryShaw
JarryShaw merged commit 52167e3 into main Oct 2, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the docs/987-protocols-vendor-statements branch October 2, 2026 17:42
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 2, 2026
JarryShaw added a commit that referenced this pull request Oct 2, 2026
Part of #987. Five quotations across two files become statements, and one
citation is corrected from authority to provenance.

sentinels.py's comment above ABSENT read "per GitHub issue #937" for the fact
that ABSENT is private by documentation rather than by underscore. That is a
what-changed statement, not a ruling attribution: #937 is the SCREAMING_SNAKE
rename, and its own body attributes the ruling to #719, which carries it. Now
written with an action verb naming the issue the change belongs to.

- Module docstring: the one-shared-module choice on #911 stated rather than
  quoted, naming what it was chosen over.
- NoDefaultType: the dedicated-class ruling and the unsettled-naming remark,
  both given in review of the work for #857, stated as what they settled.
- AbsentType: the rename ruling on #719 stated, including that documenting
  ABSENT as private replaces the underscore.
- tests/corekit: two "I prefer (2) directly" quotations become statements. The
  pull-request citations stay, since tests/ is exempt from the issue-citation
  rule per docs/source/contributing/conventions/documentation.rst:203-210, and
  the wording matches what landed for the same ruling in #991.

Three other quoted spans are left alone deliberately: two are documentation
section titles and one is a caveat the docstring makes about the code, none of
them a maintainer's words.

Prose only: token sequences identical with strings masked in both files, the
AST with docstrings blanked compares equal, and maximum line length is
unchanged at 98 and 121.
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant