docs(contributing): record the get-override rulings on the conventions page (#918) - #948
Conversation
JarryShaw
added
docs
2b98b32 to
d72a029
Compare
|
NEEDS CHANGES at The page said a delegating That third row is mine, beyond what the review reported, and it is the worst of the three: a Provenance, since it matters: the wrong claim originated as unverified analysis prose on #908 and this PR imported it unchanged. The review's sharpest point is that my own test could not catch it — it asserted the page contained the error string, never executed the shape. So the test now builds the three class shapes and runs them, and the page states all three outcomes. Also fixed, its second finding: Two notes on the review itself. Its first line carried both verdicts, which breaks the one-line contract these reviews run under — the body was unambiguous, but the verdict had to be read out of the prose. And its Sphinx build resolved Everything else it checked held: three quotes verbatim, the New revision |
JarryShaw
added
review: needs-changes
…s page (#918) Part 1 of #918: the rulings that settled what a ``get`` override owes ``EnumLookup.get`` lived only in issue and PR comments, where nothing keeps them findable. Part 2 split ``conventions.rst`` into a page per anchor so they have somewhere to land; this writes them onto the registry-protocol page. - Add *What a ``get`` Override May and May Not Do*, carrying the three rulings (#933 on ``quiet=True``, #935 on honouring an advertised signature rather than suppressing it, #940 on deleting an override that only reimplements the base) plus the ``@classmethod`` requirement, which is a language constraint rather than a ruling. Each is recorded with the reasoning, not the outcome: the code already encodes the outcome, and the reasoning is what is expensive to rediscover. - Paraphrase the rulings rather than block-quoting the maintainer, per his request on #918, and pin the page's *claim* in the tests instead of his wording. Quoting him verbatim had made an off-hand reply load-bearing in CI: an assertion required the literal ``I prefer (2) directly.`` to appear on a docs page. The quotes that predate this change are tracked in #949. - State the ``@classmethod`` constraint from measurement, on every supported version. Zero-argument ``super()`` binds the enclosing function's **first positional parameter**, whatever its name, so in ``@staticmethod def get(key, ...)`` it binds the lookup key and raises ``TypeError``. CPython words that error differently either side of 3.13 -- ``obj must be an instance or subtype of type`` on 3.10-3.12, ``obj (instance of str) is not an instance or subtype of type (Cls)`` on 3.13+ -- so the page shows both and the test asserts only ``instance or subtype of type``, which is common to them. Pinning either full sentence passes on two of the five required Compat legs and fails the other three, invisibly, since this venv is 3.14. - Record the two corollaries that make the obvious summary wrong in both directions: ``RuntimeError: super(): no arguments`` needs a function with no parameters at all, which no real override has; and an instance first argument makes the delegation **succeed silently**, so a ``@staticmethod`` override cannot be relied on to fail loudly. - Correct the audit row #940 falsified. It claimed **two** of the ``mh.py``/``ngap.py`` group define a ``get`` of their own; #940 deleted both, so the count is now none. That row rendered fine and failed nothing while naming methods that no longer exist. - Extend ``tests/project/test_conventions_doc_claims.py`` with ``GetOverrideContractTests``: four tests, ten subtests. The ``@classmethod`` test **executes** all three ``super()`` outcomes rather than grepping the page for an error string -- a prose-only assertion is what let a wrong exception stand. The ``sys.tracebacklimit`` test runs its behavioural check unconditionally instead of skipping it when the attribute is already set, which had made it degrade to a doc-text check that still reported pass. tests/project: 210 passed, 1 skipped, 572 subtests. Each new assertion was shown to fail with its claim removed; the tracebacklimit check verified to run, fail when the base raises loudly, and restore prior state with ``sys.tracebacklimit`` pre-set; and the ``TypeError`` wording measured on 3.10 through 3.14.
d72a029 to
726dd36
Compare
|
NEEDS CHANGES at The My assertion required Fixed by asserting only It also confirmed, by re-deriving from real git history rather than from the page, the two paragraphs I flagged as most likely to be resting on reasoning: #935's Two honest gaps it reported: it did not break each new executable assertion to watch it fail, and it did not rebuild the docs. I have covered the first myself. Separately, and independent of the review: the maintainer asked not to be quoted verbatim, so this revision paraphrases the four rulings and pins the page's claims rather than his sentences. That mattered functionally, not just stylistically — one assertion required the literal New revision |
JarryShaw
added
review: pending
9a21758 to
a99db83
Compare
|
NEEDS CHANGES at 1. "Labels are applied by hand, with dependabot the single exception" was false. The issue templates are a second automated path: Front-matter The review's sharper point is that my test could not see it — it asserted 2. "Three flat kind-runs" was wrong, and it was my phrasing. I wrote it in a #918 comment without measuring and it was carried onto the page verbatim: Twenty-six runs, not three — blocked for roughly the first 40% of the file, interleaved after it. The ruling is unaffected, but the characterisation was: the restructure is a regrouping of scattered entries, not a transposition of three tidy blocks. Nothing pinned it in either direction, so there is a new test. That test was vacuous on its first attempt, which I caught by running the counterfactual: it asserted Everything else it checked held: the New revision |
JarryShaw
added
review: needs-changes
a99db83 to
29c178a
Compare
|
NEEDS CHANGES at 1. The round-1 Line 766 does exactly what line 813 forbids, 45 lines apart, in the same test. The class docstring's "has nothing to bind" was wrong the same way — 2. "They fall into five groups" was false. 29 labels exist; the five groups plus 3. "Seven pre- Seven, 4. Five claims on the page were unpinned, which it proved by falsifying all five at once — 5. A docstring contradicting its own assertion — it said the run count was "pinned as a floor rather than an exact figure" while the next line required the exact figure in the prose. The docstring now says why both: the floor makes "three flat runs" unsayable, and the exact figure is meant to go red when #657 moves it, as the signal to re-measure the sentence. 6. It confirmed the rest by re-derivation, including the 26 runs and — usefully — that the "roughly first 40%" holds only by line position (run 6 starts at line 509 of 1275, 39.9%); by entry index it is 53.8%. Two minor points recorded rather than fixed: New revision |
JarryShaw
added
review: needs-changes
29c178a to
20f895c
Compare
|
NEEDS CHANGES at The blocking defect is a count I wrote. The page said
Worse, the test was named
The New test pinning what can be pinned: the changelog entry count, every module the page's And the limit stated rather than implied. Every claim whose ground truth is a GitHub query — the label count, which defaults are in live use, the Two of its findings I am not acting on, with reasons: New revision |
20f895c to
e77c640
Compare
|
NEEDS CHANGES at The red mark first:
This is the second time a local green run on 3.14 hid a 3.10 failure on this PR — round 2 was the The review's headline: 72 of 77 corruptions survived. Its classification is the useful part — (a) genuinely unpinnable offline versus (b) pinnable but unpinned — and it found the largest (b) class: ~30 link-number mismatches, where the displayed A live defect, not a corruption: And a vacuous assertion of exactly the shape I had already guarded against elsewhere. It changed the page's What I am not doing, with the reason. The remaining (b) survivors are the Audit, per Class population figures — 127 New revision |
e77c640 to
ac08400
Compare
|
NEEDS CHANGES at It found a real defect in the newest code, which is exactly where I pointed it.
And my docstring claimed it "handles exactly that shape and nothing else in TOML" — which is precisely what it did not do. That claim was the real defect; the parser was only its consequence. The review was fair that no shipped assertion reads those three keys, so nothing gave a wrong verdict today. I am fixing rather than documenting around it anyway, because Fixed by stripping New test: Everything else it checked held: the link-number test examines 44 of 44 links with none silently skipped; the anchored New revision |
ac08400 to
192f281
Compare
|
NEEDS CHANGES at
Two unpinned claims closed, and both are recurrences of shapes I have already been burned by:
Verified both bite: inverting the ruling and downgrading "The 6" each fail now, and the page is byte-restored after. On the extras reader it declined to call defects, and I agree with the judgement. Five shapes break it — single-quoted TOML strings, mixed quoting, an escaped It also confirmed the 3.10 skip is a real skip, that the test compares every key rather than the page-mentioned ones, and that a reader returning an empty dict, or only the four page-mentioned keys, or all 14 with one requirement silently dropped from Its category (a) accounting is the useful part of the 63 survivors: ~40 are genuinely unpinnable offline because their ground truth is a GitHub query and CI has no network, plus a handful whose ground truth is deleted code — historical measurements recoverable only from git. Those are not defects and I am not manufacturing pins for them. New revision |
|
NEEDS CHANGES at
Fixed by asserting Its full census of all 35
So the rule is now stated once in the code rather than rediscovered: an It also re-derived rather than trusted round 8's One thing it declined to verify, correctly: the docstring's reference to "a cross-review corrupted 46 claims" is historical narrative with no local ground truth, and the test says so itself. New revision |
192f281 to
ef7fbe9
Compare
Three settled rulings fit none of the four code-convention pages, because they govern the repository rather than the library. The owner ruled on #918 that they get a fifth page rather than staying in their threads. - Add ``docs/source/contributing/conventions/process.rst`` (``.. _process:``), covering what the ``all`` extra carries (#910), what a changelog entry is, and what the issue and pull request labels mean. Paraphrased throughout rather than quoting the owner, on his instruction on the same issue. - Record the changelog grouping as ruled: a section per top-level module with ``Added``/``Changed``/``Fixed`` nested inside each. The restructure belongs to #657, which owns the file and merges last. The one case the rule does not settle, an entry spanning modules, is flagged rather than decided. - Describe the file's **current** shape from measurement. An earlier draft called it "three flat kind-runs", carried from a comment nobody had checked; the 80 entries actually carry the three kind labels in **26** runs. - Document the label scheme the owner asked for alongside ``breaking``, and name **both** automated paths: dependabot, and the issue templates, which apply a label from front matter before anyone reads the issue. An earlier draft called dependabot the single exception. - Correct two ``breaking`` claims measurement contradicts. It **is** applied below #350 -- carriers ``#3``-``#28``, nothing between ``#28`` and ``#350``, issues from ``#775``. And no ruling defines the label, so the page states the behavioural test rather than attributing a phrasing to anyone. - Correct the audit table's population to **six** -- the ``mh``/``ngap`` helpers ``EnumLookup`` subclasses those modules define, ``ProcedureCode``/``ProtocolIE`` being re-exports. The page had carried three figures for one population. Tests, and the reason each exists rather than the shape of it: - ``ProcessConventionTests`` pins the page's claims against the tree. Two were rewritten because they could pass for the wrong reason: one read only ``dependabot.yml`` and never ``ISSUE_TEMPLATE/``, and one asserted the bare string ``26``, which the page's own ``wc -l`` output satisfied. - ``test_a_non_string_key_raises_a_value_miss_on_every_helper`` **derives** the population instead of listing it. Its predecessor was named ``..._on_all_seven`` and exercised three. - ``test_every_issue_link_number_matches_its_own_url`` closes the largest class of unpinned claim: a cross-review corrupted ~30 link numbers and not one was caught, because nothing compared the displayed ``#NNN`` against the number in its own URL. Invisible to a reader, unwarnable by Sphinx. - ``_optional_dependencies`` replaces ``tomllib``, which is 3.11+ and broke the ``Python 3.10`` leg while passing locally on 3.14 -- ``tomli`` is not a dependency either, so there was nothing to fall back to. It now strips comments and tracks bracket **depth**: a version matching to the first ``]`` mis-parsed **3 of the 14** extras silently, emptying ``vendor`` on the ``]`` inside ``"requests[socks]"``, truncating ``dev``, and **inventing a requirement called** ``9 skipped`` **out of comment prose**. No shipped assertion read those keys, but ``dev`` and ``vendor`` are both described in the page's prose, so the next assertion to check either would have got wrong data with nothing raised. ``test_the_extras_reader_agrees_with_tomllib`` now compares the two key for key wherever a real parser exists, and skips on 3.10 rather than pretending to. - The page states plainly what cannot be pinned here: any claim whose ground truth is a GitHub query, since CI has no network. The command beside the figure is the pin, run by a reader rather than by CI. tests/project: 221 passed, 1 skipped, 640 subtests. The extras reader verified byte-identical under python3.10 and 3.14, and every new assertion shown to fail with its claim removed or its defect reintroduced.
ef7fbe9 to
e4340e8
Compare
|
NEEDS CHANGES at 1. The page attributed a hand-applied label to dependabot — inverting the section's own point. Verified: Only That claim had no pin at all, which is how it survived. It does now: the test derives the configured ecosystems and asserts the page does not re-attribute 2. An assertion that could never fail. 3. A test with no floor. The censuses, which are the durable half:
It also caught a pytest-subtests undercount while doing it: pytest reported New revision |
|
GOOD TO GO at The check that matters for the merge decision, re-derived myself rather than relayed. Both edit sets compose: three One thing I nearly mis-reported: Its It also verified the three assertions One finding I am not acting on, with the reason. It also noted the new
Unpublished and yours to merge. |
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
…view The by-module restructure rewrote ``docs/source/changelog/1.5.0.rst`` wholesale, which conflicted with #946's one-word anchor fix on ``main``. Resolved by taking the restructured file and applying that fix to it (``959#section-5.3`` -> ``959#section-5``), then regenerating ``CHANGELOG.md``. Merging ``main`` brings #948's tests, which the restructure makes **vacuous** rather than merely stale, so they are rewritten rather than renumbered: * ``test_the_page_describes_the_changelog_kind_runs_as_they_are`` measured the inline kind labels the restructure removed: its ``findall`` matched nothing, ``runs`` was empty, and both ``assertGreater`` floors failed before the prose needle. Replaced by ``..._grouping_as_it_is``, which counts the ``-`` and ``~`` underlined sections, requires the kinds nested inside the modules, requires no inline label anywhere, and requires the section count in the page's prose. * ``test_the_changelog_file_is_not_shaped_one_entry_per_commit`` and ``test_the_page_pins_its_own_measured_numbers`` both keyed on ``^\* \*\*Kind\*\*``; an entry is now a column-zero bullet. * ``process.rst`` described the file as "neither shape", with figures for a layout that no longer exists. It now describes what shipped: 9 module-level sections holding 155 entries, no inline kind labels. Cross-review findings on ``a84020c3a``, both confirmed before fixing: * The stated reason for retargeting ``test_a_sub_heading_underline_joined_into_the_prose_is_fatal`` was backwards, in the commit message and in the test's own comment. ``_reject`` is built on ``assertRaises``, so the old ``-`` input converting cleanly made that test **fail**; the retarget was required, not a tidy-up. Comment corrected. * ``tests/project/test_isort_clean.py`` cited ``1.5.0.rst`` lines 691 and 1169 -- already stale by 2 and 18 before this work, and off by ~2700 after the reshuffle. Line numbers dropped; the file is cited alone. * Added ``test_an_over_long_sub_heading_underline_is_fatal``: ``_RESIDUAL``'s comment now claims ``=``, ``-`` and ``~`` all need to stay in its alternation, and only ``=`` was pinned. ``pytest tests/project``: 225 passed, 1 skipped, 660 subtests, 0 failed; ``unittest`` agrees at 226 tests OK. Changelog drift gate exit 0.
1 participant
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 — changelog 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 1 of #918. Part 2 merged as
d7489c61cand created the page-per-anchor directory; this writes theget-override rulings into it.Three rulings harvested onto registry-protocol, each with its quote and the reasoning rather than the outcome — the code already encodes the outcome:
BaseErrorsetssys.tracebacklimitprocess-wide (#362), so uniformity beat the per-class argumenttype: ignore[override]plus a docstring is not an answer to a contract the class advertises and breaksThe trap behind #940 is recorded because it is what a future override will hit: the base branches on
isinstance(key, str)and treats the rest as a value, while the deleted overrides branched onisinstance(key, int)and fell to the name path — soget(None)was a quietEnumKeyErroron two classes and a loudEnumValueErroron the other five, with no prose anywhere saying so.Also corrects an audit row #940 falsified: it claimed two of that group define their own
get; both were deleted, so the answer is now none.pcapkit/protocols/internet/mh.pycontains zerodef get(. That row rendered fine and failed nothing.The
@classmethoditem is deliberately not presented as a ruling. It is a language constraint — zero-argumentsuper()in a@staticmethodhas nothing to bind — with #913 setting the shape and #908 following it. Worth flagging because the first draft of this change did quote it as a maintainer ruling, attributing a sentence that is actually from analysis prose on #908; every comment on this repo carries the same author, so that distinction has to be made by reading, not by author field.Not included, and the reason #918 cannot close on these four pages alone: three settled rulings fit none of them — the
allextra being core addons only (#910), one commit per changelog entry, and thebreakinglabel's meaning. They are process rather than code conventions. Asked separately on #918 rather than inventing a fifth page.Test.
GetOverrideContractTests— 4 tests, 10 subtests — pins the page's wording and the tree it describes:vars(Method)['get']is aclassmethodwhileCommand/OptionTypearestaticmethods whose source has nosuper(); a name miss leavessys.tracebacklimitunset;'get' not in vars(cls)for bothmhclasses;get(None)andget(1.5)raiseEnumValueError.tests/project: 210 passed, 1 skipped, 572 subtests (206 + 4 before). Each assertion was shown to fail with its claim removed from the page.UNVERIFIED: Sphinx rendering — no
make htmlrun. The section adds no new:ref:/:doc:targets, only roles already used on the same page.