docs(corekit): state the sentinel rulings instead of quoting them - #992
Conversation
JarryShaw
added
docs
|
NEEDS CHANGES at Six maintainer quotations still survive in The cause is a count in this issue that is wrong, and a pass that implemented the list instead of measuring. #987 says there are "two further" One reading worth recording, because it would otherwise be used to dismiss this. The Also found: a length filter is what hid Verified otherwise, and the citation fix came out better than I credited it. Fix dispatched. Label stays |
JarryShaw
added
review: needs-changes
The cross-review on #992 found six maintainer quotations surviving in tests/corekit/test_enum_lookup_reparent_930_unit.py, one pair inside the very docstring whose second paragraph the first pass had already converted -- so a single __doc__ showed both forms fifteen lines apart. The first pass implemented #987's remainder list rather than measuring. That list said two "I prefer (2) directly" quotations; grep over main gives three, at lines 33, 202 and 454, and only the last two were converted. The issue body is corrected, and the scan this time sets no minimum span length -- one of the six was two characters long and a length filter is what hid it. - The #935 lean and the two #940 quotations at :29-33 become one statement of what each settled, keeping the pull-request citation. - #933's reversal is paraphrased differently at the two sites that draw on it, because they make different points: the module docstring takes what the ruling settled, while the class docstring takes why an earlier revision of this file had pinned the opposite. - The prose said the follow-up came four minutes after the first answer. The comments are 21:32:21Z and 21:37:17Z, so 4m56s; now "a few minutes later", which is what the evidence carries. The pull-request citations stay. tests/ is exempt from the name-the-issue rule per docs/source/contributing/conventions/documentation.rst:203, but that exemption does not reach the no-verbatim rule at :184, which has no carve-out. Prose only: token sequences identical with strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, and maximum line length stays 121. A re-scan finds zero maintainer quotations.
9c8dc36 to
b47b9b4
Compare
|
Fixed and pushed as The two #933 sites got different paraphrases rather than one copied twice, which is the right call and not what I asked for: the module docstring states what the ruling settled, while the class docstring states why an earlier revision of this file had pinned the opposite. They were drawing different points from the same comment. It also found a factual error beyond its brief, which I verified: the prose said the follow-up ruling came four minutes after the first answer. The comments are 21:32:21Z and 21:37:17Z — 4m56s. Now "a few minutes later", which is what the evidence actually carries rather than a precision it cannot. Verified by me rather than relayed: token sequences identical with every string masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, maximum line length stays 121, and no assertion depended on any of the quoted text. The Label back to |
JarryShaw
added
review: pending
|
GOOD TO GO at The two #933 sites describe the same event incompatibly. The class docstring dissolves a fact into a perception. It says the first answer "was read as keeping them loud … on the strength of that reading". I verified why that is wrong: #933's body pre-labels its own option 2 as a refusal — line 15 reads Also going in: "five minutes later" restored in place of "a few minutes", since 4m56s rounds to five and this file is otherwise precise about numbers. One correction to my own brief, which the review caught. I told the worker that Verified otherwise, with the scan this time independent of any count: four passes plus a search for thirteen known maintainer utterances and for emphasis-wrapped quote spans — zero hits, and exactly the two legitimate survivors. Prose-only reproduced exactly (271 and 2800 tokens, sequences identical, ASTs equal with docstrings blanked), all six differing string tokens are docstrings, no |
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.
The cross-review on #992 found six maintainer quotations surviving in tests/corekit/test_enum_lookup_reparent_930_unit.py, one pair inside the very docstring whose second paragraph the first pass had already converted -- so a single __doc__ showed both forms fifteen lines apart. The first pass implemented #987's remainder list rather than measuring. That list said two "I prefer (2) directly" quotations; grep over main gives three, at lines 33, 202 and 454, and only the last two were converted. The issue body is corrected, and the scan this time sets no minimum span length -- one of the six was two characters long and a length filter is what hid it. - The #935 lean and the two #940 quotations at :29-33 become one statement of what each settled, keeping the pull-request citation. - #933's reversal is paraphrased differently at the two sites that draw on it, because they make different points: the module docstring takes what the ruling settled, while the class docstring takes why an earlier revision of this file had pinned the opposite. - The prose said the follow-up came four minutes after the first answer. The comments are 21:32:21Z and 21:37:17Z, so 4m56s; now "a few minutes later", which is what the evidence carries. The pull-request citations stay. tests/ is exempt from the name-the-issue rule per docs/source/contributing/conventions/documentation.rst:203, but that exemption does not reach the no-verbatim rule at :184, which has no carve-out. Prose only: token sequences identical with strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, and maximum line length stays 121. A re-scan finds zero maintainer quotations.
Round 2's review found two accuracy defects in the paraphrases this pull request had just written, and they pull in opposite directions from the one this issue usually catches. The class docstring said the owner's first answer "was read as keeping them loud", and that an earlier revision pinned loud "on the strength of that reading". That dissolves a fact into a perception and puts the error on the reader. #933's body pre-labels its own option 2 as a refusal -- a get_all miss being a genuine failure the caller should see loudly -- so the bare answer selected a numbered option that already meant keep-loud. There was no other reading available. - The first answer is now stated as a bare refusal selecting the issue's own numbered option, with the file's earlier revision pinning loud on that basis. - The two sites described the same event incompatibly, one as a reversal and one as a clarification. "I meant" may say something about intent, but intent is not recoverable from the thread; the effect on the recorded ruling was a reversal, and #933's own recording comment leads with that word. Both sites now say reversed. - "a few minutes later" becomes "five minutes later". 21:32:21Z to 21:37:17Z is 4m56s, and this file is otherwise precise about numbers. - The redundancy comparison preceded the ruling and the 20-call-site check did not, so the two are no longer described as one measurement. The call-site timing is left neutral because the thread does not settle it. Prose only: token sequences identical with strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, and maximum line length stays 121.
b47b9b4 to
9a1779e
Compare
|
Fixed and pushed as Both accuracy defects are out. The first answer on #933 is stated as what it was — a bare refusal selecting the issue's own numbered option for keeping them loud — rather than as something that "was read as" keeping them loud, and the file no longer calls the same event a reversal in one place and a clarification in another. The worker settled the verb on evidence rather than preference: intent is not recoverable from the thread, whereas the effect on the recorded ruling is, and #933's own recording comment leads with that word. "five minutes later" restored, and the two measurements separated — the redundancy comparison preceded the ruling, the 20-call-site check did not, and its timing is left neutral because the thread does not settle it. That last restraint is right: the alternative was inventing an order to make the sentence read better. Verified by me: Label stays |
|
NEEDS CHANGES at
Result and dispatch in one comment, so the check provably preceded the acting — and that holds on both branches of the remaining ambiguity. The base text had said "Measured before acting on that final ruling" as one sentence covering both halves, which was accurate for both. Splitting them was the right instinct, since the alias comparison is provably pre-ruling while the grep is only provably pre-acting; the new wording just made the second half false. The fix is one word, keeping the split. Two things the review settled that I had flagged as risks, and both came back against me: The over-correction risk on the #933 refusal does not materialise, and the evidence is stronger than the fix claimed. #933's body poses a direct yes/no question and labels its branches "Reversed" is not merely defensible but required. Calling it a clarification would imply the first answer was unclear or misread — which is the exact defect round 3 had just removed, since the answer was determinate. So "clarified" would have reintroduced it two paragraphs later. Also non-blocking and going in with the one-word fix: Verified otherwise: 271 and 2800 tokens, sequences identical, ASTs equal with docstrings blanked, and every differing string token checked as a docstring on both sides rather than only at head — zero non-docstring string differences, so nothing moved that could change behaviour. The scan found exactly the two legitimate survivors and no third. Rebase clean, three commits, nothing arrived from |
JarryShaw
added
review: needs-changes
Round 3's review found the previous round had replaced a true sentence with a false one. Splitting the two measurements was right -- the alias comparison is provably pre-ruling, the 20-call-site grep only provably pre-acting -- but "Checked when acting on it" asserts an ordering #940 contradicts: - 01:57:57Z the grep is promised, unconditionally, to be reported before anything is touched; - 02:00:58Z the ruling lands; - 02:01:29Z one comment carries both the grep's result and the announcement that the rewrite is being dispatched. Result and dispatch in the same comment, so the check preceded the acting on every reading: if the grep ran in the three-minute wait it was before the ruling too, and if it ran in the following 31 seconds it still preceded the dispatch. The base text had said "Measured before acting on that final ruling" across both halves, which was accurate for both. - "when acting on it" becomes "before acting on it", keeping the split. - Re-flow the paragraph at 91 columns. One line had been left at 107 where its neighbours run 73-91, the residue of a 179-character line caught mid-edit that came back under the limit without the paragraph being re-wrapped. Lines over 95 characters go from two to one, and the survivor is pre-existing. Prose only: token sequences identical with strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, and maximum line length stays 121.
|
Fixed and pushed as The word is corrected to "before acting on it", keeping the split the previous round had introduced, and the 107-character line is gone: lines over 95 characters go from two to one, and the survivor is pre-existing and untouched. Verified by me at Prose-only re-checked by me: 2800 tokens identical with strings masked, the AST with docstrings blanked compares equal, both differing string tokens are docstrings. This round is a good illustration of why the verdict is worth more than the diff. The blocking defect was introduced by the previous round's fix, which had been dispatched to improve accuracy and did improve it in two places while quietly breaking a third — and the sentence it broke had been correct on Label stays |
Round 3's review found the parenthetical this pull request added asserting the opposite of what #842 says, and that instruction was mine. #842 carries two AppType exceptions, not one: the issue body has the routing exception, and comment 5852772329 answers whether the generic register_alias requires an existing member with "always, unless AppType and the concrete enumerations need to call it on non-existing members". That unless is a carve-out on exactly the question the parenthetical said the issue was not about. main's text was lossy by omission -- it carried the first half of that sentence and dropped the unless. The parenthetical turned the omission into a positive false claim, which is worse, and is the third way this pull request has lost the same qualifier: round 1 invented an exception no registry implements, round 2 flattened a modal into a universal, round 3 denied the hedge existed. - The passage now states what #842 settled and what it left open, then answers the open part from behaviour: AppType's override requires the port to carry a member of that registry already. It names the concrete enumerations too, which the comment does and my instruction had dropped. - "stricter still" is gone rather than qualified. The base tests membership in _value2member_map_ and AppType tests __registry__.getlist(port), so it was never a strengthening of one predicate. - Two untouched lines said every generated enumeration under pcapkit.const inherits from EnumRegistry, which the sentence added last round contradicts. Both now say registry: the three closed sets are generated and inherit EnumLookup, so "generated" was no escape hatch. - The paragraph is re-flowed to 75-82 columns. The edit had left a 52-column line whose successor fit, the same defect #992 was sent back for. Prose only, at the strictest setting: 753 tokens identical with no exclusions at all and only strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, maximum line length stays 96. The four enum test files: 130 passed, 112 subtests.
|
GOOD TO GO at I asked whether "before acting on it" now claims less than the record supports, since the sentence deliberately avoids saying "before the ruling". It does not, and the evidence runs the other way: the grep is promised in the future tense at 01:57:57Z — to be reported before anything is touched — which is positive evidence it had not yet run at that point. So the record's only bound is the interval from 01:57:57Z to 02:01:29Z, and that interval straddles the 02:00:58Z ruling. "Before the ruling" would therefore be an over-claim, and the 31-second gap between ruling and dispatch is an inference about timing rather than a timestamp a docstring may assert. The hedge is load-bearing, not padding. It also confirmed the self-report does not help: the "all measured before you ruled" phrase governs the paragraph it opens, and the 20-call-site result sits in the next one. The re-flow changed exactly one word in the entire module docstring. I verified that independently: 1189 words before and after, a single diff opcode, Also verified: no literal or role split across a line break, checked by regex and by parsing the paragraph through One thing recorded for a later pass rather than fixed here: Labelling |
JarryShaw
added
review: good-to-go
Round 3's review found the parenthetical this pull request added asserting the opposite of what #842 says, and that instruction was mine. #842 carries two AppType exceptions, not one: the issue body has the routing exception, and comment 5852772329 answers whether the generic register_alias requires an existing member with "always, unless AppType and the concrete enumerations need to call it on non-existing members". That unless is a carve-out on exactly the question the parenthetical said the issue was not about. main's text was lossy by omission -- it carried the first half of that sentence and dropped the unless. The parenthetical turned the omission into a positive false claim, which is worse, and is the third way this pull request has lost the same qualifier: round 1 invented an exception no registry implements, round 2 flattened a modal into a universal, round 3 denied the hedge existed. - The passage now states what #842 settled and what it left open, then answers the open part from behaviour: AppType's override requires the port to carry a member of that registry already. It names the concrete enumerations too, which the comment does and my instruction had dropped. - "stricter still" is gone rather than qualified. The base tests membership in _value2member_map_ and AppType tests __registry__.getlist(port), so it was never a strengthening of one predicate. - Two untouched lines said every generated enumeration under pcapkit.const inherits from EnumRegistry, which the sentence added last round contradicts. Both now say registry: the three closed sets are generated and inherit EnumLookup, so "generated" was no escape hatch. - The paragraph is re-flowed to 75-82 columns. The edit had left a 52-column line whose successor fit, the same defect #992 was sent back for. Prose only, at the strictest setting: 753 tokens identical with no exclusions at all and only strings masked, both differing string tokens are docstrings, the AST with docstrings blanked compares equal, maximum line length stays 96. The four enum test files: 130 passed, 112 subtests.
JarryShaw
removed
the
review: good-to-go
1 participant
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible — N/A, centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything elseDescription of your pull request and other information
Part of #987. Five quotations across two files become statements, and one citation is corrected from authority to provenance.
The citation defect.
sentinels.py's comment aboveABSENTread "per GitHub issue #937" for the fact thatABSENTis private by documentation rather than by a leading underscore. #987 names this one specifically, and it is a what-changed statement rather than a ruling attribution: #937 is the SCREAMING_SNAKE rename, and its own body attributes the ruling to #719, which carries it in a comment of 2026-09-30T00:24:10Z. Rewritten with an action verb naming the issue the change belongs to.The quotations. The one-shared-module choice on #911; the dedicated-class ruling and the unsettled-naming remark, both given in review of the work for #857; the rename ruling on #719; and two
I prefer (2) directlyquotations undertests/corekit. Each is now a statement of what was settled, with enough surrounding context that the ruling stays findable — which is the convention the maintainer restated on #987 while this was in progress.Two judgement calls worth stating. The
tests/pull-request citations stay, becausetests/is exempt from the issue-citation rule perdocs/source/contributing/conventions/documentation.rst:203-210; and the two#857rulings keep that number rather than gaining#859, the pull request the review actually happened on, because the guidance is more context rather than a more precise citation. Three further quoted spans are deliberately untouched: two are documentation section titles and one is a caveat the docstring makes about its own code, none of them a maintainer's words.Prose only, verified rather than asserted. Token sequences are identical with every string masked, the AST with docstrings blanked compares equal, and maximum line length is unchanged at 98 and 121.
tests/corekitgives 400 passed, 16 skipped, 658 subtests.