Skip to content

docs(contributing): paraphrase the quoted rulings, and pin claims not wording (#949) - #951

Merged
JarryShaw merged 1 commit into
mainfrom
docs/949-paraphrase-quoted-rulings
Sep 30, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/949-paraphrase-quoted-rulings

Conversation

@JarryShaw

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

Description of your pull request and other information

Closes #949.

The convention pages recorded the maintainer's design rulings by block-quoting him verbatim, and several tests asserted those quoted sentences as literal strings. Two problems: the pages read like a transcript, and his casual phrasing in a thread was load-bearing in CI.

Twenty quotes paraphrased across the five pages, attribution kept by issue number. grep -n verbatim over them now returns exactly one hit — forwards ``default`` verbatim at registry-protocol.rst:312, the legitimate word use. Deliberately kept verbatim: RFC and IANA spec quotations, whose words are normative, and the AbsentType docstring block quote, which is the package's own text cited with file:line rather than a thread reply.

The tests now derive claims instead of pinning sentences. The seven audit population figures (127 registries, 124 files, 117 int-valued, 5 flag, 10 StrEnum, 24 non-registry, 151 total) are read out of the page and compared against a runtime walk; R1_Counter = 128 and R1_COUNTER = 129 are pinned and executed through Parameter.get, with an assertNotEqual that they have not collapsed.

Two robustness defects fixed while in here:

  • vars(Method)['get'] and its two siblings were indexed unguarded, so folding an override into the base — the shape fix(protocols): widen the two kept get overrides in mh.py to accept default #940 created for the mh/ngap helpers — raised a bare KeyError('get'). Now an AssertionError naming the cause.
  • The dependabot label assertion pinned one phrasing, so a reworded violation walked past it. Rebuilt as a set-comparison against package-ecosystem in dependabot.yml. Verified on six cases: five violation shapes go red, none of which tripped the old needle, while a harmless rewording stays green.

pytest tests/project: 225 passed, 1 skipped, 670 subtests, 0 failed. Re-derived with plain unittest as well (226 tests, OK), since pytest-subtests can report a parent as passed when only its subTests fail. One assertion was falsified by hand as a spot check — altering a page figure reddens the matching subtest and nothing else.

Not verified: no local Sphinx build, so rendering is unproven. Two roles newly introduced on registry-protocol.rst already appear on that same page, and every project test passes, but the docs job on this PR is the real check.

@JarryShaw JarryShaw added docs Pull requests that change documentation only (docs: subject prefix) test Pull requests that add or correct tests (test: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 30, 2026
@JarryShaw
JarryShaw force-pushed the docs/949-paraphrase-quoted-rulings branch from 043970d to 35169c7 Compare September 30, 2026 20:11
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 043970d79 — sonnet cross-review, round 1. Both findings confirmed by me and fixed at 35169c760.

1. A verbatim quote the sweep missed, in pyproject.toml:231: the four capture engines were described as "on demand for each one" in scare quotes. I checked #910's comments myself — he wrote that phrase. It escaped because it carries no verbatim marker and sat outside the hunks this change already touched, two sentences from a quote the same diff did paraphrase. Now reworded to "installed one at a time on demand".

2. The vars(...)['get'] guard did not replace the bare KeyError — it ran alongside it. The membership loop was wrapped in self.subTest(), which records a failure and lets the method continue, so the unguarded indexing four lines down still raised KeyError: 'get'. Verified by deleting Method.get to recreate #940's shape: the old form reported 1 failure and 1 error, the fix reports 1 failure, 0 errors. The subTest is gone, with a comment saying why, since re-adding it would silently restore the defect.

3. Also fixed, unprompted: the review found the dependabot clause used re.search, so a page carrying two attributing clauses — one right, one wrong — was checked only on the first. Demonstrated concretely: with a second clause naming github_actions, search read {dependencies, python} and passed; findall reads both and fails. That is the same walk-past-the-check defect #949 exists to remove, so it was not worth leaving as an edge case.

pytest tests/project: 225 passed, 1 skipped, 667 subtests, 0 failed, and unittest agrees (226 tests, OK). Subtests drop 670 → 667 because three membership checks are no longer subtests.

The docs question is now settled rather than open. The PR body said rendering was unverified. deploy-pages did build it — on pull/951/merge, make -C docs html → build succeeded, 45 warnings, which is exactly main's count, with no warning naming any conventions page. So the paraphrasing adds no docs warnings.

Two things the review reported against itself, worth recording: it spawned a background agent despite its brief forbidding it, and said so; and it marked two assertions UNVERIFIED rather than claiming it had checked them. Re-review requested at the new head.

@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 Sep 30, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 35169c760 — sonnet cross-review, round 2. All three round-1 fixes hold.

I re-derived the one claim I had not tested myself: the review asserted the guard now works for all three classes, not just Method. Deleting each of Command.get and OptionType.get in turn gives 1 failure, 0 errors, and the message names the offending class in both cases. So the fix is general, not accidental.

On the other two: the pyproject.toml paraphrase preserves both halves of the ruling (per-engine, on demand) with no drift, and a re-sweep of that whole file found no other quote of his — the three remaining double-quoted strings are a PEP 440 behaviour description, literal pytest output, and literal YAML. The dependabot findall loop caught four attack shapes including wrong-then-correct order and three clauses; the review could not get a violation past it.

Subtest arithmetic accounted for: 670 → 667 is exactly the three subTest wrappers removed from the guard loop. Nothing else in the diff can move the count, since pyproject.toml is comment-only and the dependabot loop was never wrapped.

Two limitations named rather than papered over. The guard loop iterates a fixed tuple with a plain assertIn, so if two classes lost their get in one change only the first is reported — the correct consequence of aborting early, but it means a double regression takes two cycles to surface fully. And the dependabot check is keyed on one sentence shape, so a false attribution written in a wholly different structure is still outside its reach; that is a pre-existing property of any keyed-clause regex, partially covered from the other side by the hand-applied assertion.

CI at this head: 53 ok / 0 fail / 5 still running, base bb3562e02 which is current main, so no rebase needed. MERGEABLE/BLOCKED is only those 5 outstanding legs.

Unpublished and yours to merge once the last legs land — I will say so on the next pass rather than calling it ready while checks are in flight.

… wording (#949)

The convention pages recorded design rulings by block-quoting the maintainer
verbatim, and several tests asserted those sentences as literal strings -- so a
casual reply in a thread was a CI build dependency.

* Paraphrase all verbatim quotes across the five convention pages and the two
  ``pyproject.toml`` comment blocks, keeping attribution by issue number. Spec
  quotations (RFC/IANA) and the package's own cited docstring stay verbatim,
  since those words are normative.
* Replace the assertions that pinned his sentences with ones that derive the
  claim from the tree: the seven audit population figures, the
  ``R1_Counter``/``R1_COUNTER`` values executed through ``Parameter.get``, and a
  stray-registry check.
* Guard ``vars(Method)['get']`` and its two siblings with a membership check, so
  folding an override into the base reports an ``AssertionError`` naming the
  cause instead of a bare ``KeyError`` -- the shape #940 created. Deliberately
  not under ``subTest``, which would record the failure and let the bare
  ``KeyError`` be raised anyway by the indexing below it.
* Rebuild the dependabot label assertion as a derivation from
  ``package-ecosystem`` in ``dependabot.yml``, replacing a single-phrasing
  ``assertNotIn`` that a reworded violation walked straight past. Every clause
  matching the shape is checked, not just the first.
* ``tests/corekit/test_sentinel_exports_unit.py`` asserted the literal ``ONLY``
  -- the maintainer's capitalisation, lifted from inside the #911 block quote
  this change paraphrases. It now checks the export boundary against
  ``pcapkit.corekit.sentinels.__all__`` and requires only the identifier and the
  issue number on the page.

``pytest tests/project``: 225 passed, 1 skipped, 667 subtests, 0 failed.
``tests/corekit`` doc-reading files: 61 passed, 96 subtests, 0 failed.
@JarryShaw
JarryShaw force-pushed the docs/949-paraphrase-quoted-rulings branch from 35169c7 to 675ad67 Compare September 30, 2026 20:33
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Sep 30, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

CI went red at 35169c760 — four legs, one defect, and my brief caused it. Fixed at 675ad6703.

Python 3.11/3.12/3.13/3.14 all failed on the same single test, so this is one fault reported four times rather than four faults:

FAILED tests/corekit/test_sentinel_exports_unit.py::SentinelPopulationTests::
       test_conventions_doc_records_that_only_the_object_is_exported
AssertionError: 'ONLY' not found in ...

Root cause. That test asserted the literal string ONLY on sentinel-convention.rst. At main:81 the page read What reaches users is the **object only**. The maintainer's ruling: *"we should ONLY … — so the assertion was pinning his own capitalisation from inside a verbatim block quote, which is precisely the defect this PR removes. Paraphrasing the quote deleted the word and the test went red.

Why it escaped the local run: I scoped the worker to pytest tests/project and never told it that files outside that directory read the convention pages. That was my omission, not the worker's.

I censused it rather than fixing just the one: of the ten test files mentioning a conventions path, only tests/corekit/test_sentinel_exports_unit.py actually reads one outside tests/project — the two tests/protocols hits and test_fields_numbers_unassigned_enum.py only cite the path in prose, so paraphrasing cannot affect them.

The fix pins the claim, not the casing. The export boundary is now checked against pcapkit.corekit.sentinels.__all__ — every public sentinel's instance is in it, and its type is not — and the page is required only to carry __all__ (an identifier) and #911. Proved falsifiable: leaking NullType into __all__ turns it red.

tests/project 225 passed / 1 skipped / 667 subtests; the three tests/corekit doc-reading files 61 passed / 96 subtests, 0 failed.

review: good-to-go pulled back to review: pending — the verdict was given at 35169c760 and this is a new, unreviewed change. Python 3.10 had not finished when the others failed; it will re-run on this head.

@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 Sep 30, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 675ad6703 — sonnet cross-review, round 3. Scoped to the one file that changed since round 2's clearance, confirmed by git diff 35169c760 HEAD --stat: only tests/corekit/test_sentinel_exports_unit.py, +37/−2.

The replacement assertion is falsifiable and strictly stronger than what it replaced. The review reproduced my own check independently — adding NullType to pcapkit.corekit.sentinels.__all__ yields SUBFAILED(sentinel='NULL') with the intended diagnostic — and could not construct a state where #911 is genuinely violated while the test stays green. The old assertIn('ONLY', section) asserted only that an uppercase word appeared somewhere in the section, tied to no claim; the new form checks the export boundary against the module and asks the page for an identifier and an issue number.

It also caught that my census method was narrower than I said it was. I searched tests/ for the word conventions; searching for the page slugs instead finds four more files — tests/const/test_const_registry_protocol.py, test_const_enum_builtin_parity.py, test_const_enum_get.py, tests/vendor/test_ipx_socket_unit.py — which name a page as mint-criterion without ever writing "conventions". So "ten files" was an artefact of my pattern, not a fact about the tree.

I checked those four rather than taking either of us on trust, because a crude read_text|Path( count said they do read files, contradicting the review's "no read". Both are right in the end: every read in them targets Python source (pcapkit/const/ipx/socket.py, generated modules), never an .rst, and their slug mentions are all docstring prose. None pins an owner phrase. So no further exposure — but the lesson is that a docs-blast-radius census keyed on the directory name misses files that cite a page by slug.

CI at this head: 51 ok / 0 fail / 7 still running, with the previously-failing sentinel test now passing. Base is bb3562e02, current main, so no rebase.

Unpublished and yours to merge once the last legs land — I will confirm on the next pass rather than call it ready with checks in flight.

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

Status: Done

Development

Successfully merging this pull request may close these issues.

docs(contributing): paraphrase the quoted rulings on the conventions pages, and stop asserting them as literal strings

1 participant