From df87cb33b3c1c856a1bbe541ee961a10fa5028cb Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 29 Sep 2026 15:44:17 -0400 Subject: [PATCH] docs(conventions): harvest the settled rulings, correct the stale prose (#918) * Retitle *Registry Conventions* -> *House Conventions* and widen the preamble: the page now carries a protocol-class ruling as well as `pcapkit.const` ones, and records the standing ask that a ruling is written here in the same change that implements it. * Correct the five passages #927 left for this issue: the `FEATCode` name miss raises `EnumKeyError` rather than a bare `KeyError`; a `_validate_value` rejection propagates unwrapped with no usable `default`; #877's phase 2 has landed for 17 of the 24 non-registry enumerations rather than "not happened yet"; `TransportProtocol.get` is now only a case fold and `Criticality.get` is gone. * New "What a Failed Lookup Raises" for #923's provenance-and-shape ruling. * New "Which bases an IPv6 extension header names" for #924's subclassing ruling, the RFC census behind it, the retired `IPv6_GenericExt` name, and why ESP is an extension header that still cannot short-circuit the chain walk. * Document `EnumValueError`, which had no `autoexception` entry, so five references to it on this page rendered as plain text. 15 new tests pin the checkable claims. tests/project 193 OK, test_sentinel_exports_unit 18 OK, test_ipv6_ext_unit + FEATCode + enum-lookup-base 77 OK; docs build clean, every new cross-reference resolved in the rendered HTML. --- docs/source/contributing/conventions.rst | 337 ++++++++++++- docs/source/pcapkit/utilities/exceptions.rst | 4 + .../test_const_ftp_featcode_case_903_unit.py | 6 +- tests/project/test_conventions_doc_claims.py | 450 ++++++++++++++++++ 4 files changed, 777 insertions(+), 20 deletions(-) create mode 100644 tests/project/test_conventions_doc_claims.py diff --git a/docs/source/contributing/conventions.rst b/docs/source/contributing/conventions.rst index 0f7634696..9bccf2f67 100644 --- a/docs/source/contributing/conventions.rst +++ b/docs/source/contributing/conventions.rst @@ -1,13 +1,27 @@ -Registry Conventions -==================== +House Conventions +================= .. important:: - This page records **design rulings** for :mod:`pcapkit.const` -- decisions that + This page records **design rulings** for :mod:`pcapkit` -- decisions that are not derivable from the code, and that a future maintainer or an automated contributor would otherwise have to rediscover by reading a closed issue thread. Each ruling names where it was settled. + Most of them govern :mod:`pcapkit.const`, which is where the settled questions + have mostly arisen, and the page was titled *Registry Conventions* for that + reason until `#918 `__ + widened it. :ref:`extension-header-subclassing` is the first ruling here that + governs a protocol class hierarchy rather than a registry. + + **A ruling that stays in its thread is a ruling that gets rediscovered.** So + when a question is answered in a way the code cannot express on its own -- a + classification, a naming rule, a deliberate asymmetry -- it is written onto + this page in the same change that implements it, rather than being left in the + issue for the next contributor to find. The owner's standing ask, on + `#918 `__: *"And any future + conventions to be settled - document them as well."* + .. _mint-criterion: When an unrecognised value may mint a member @@ -353,6 +367,18 @@ Three things about it are easy to get wrong: it subclasses :exc:`ValueError` a rejection is caught by ``get``'s own ``except`` and falls back to ``default`` like any other unresolvable value. An override raising outside that hierarchy propagates past ``default`` instead. + + With **no usable** ``default``, the rejection reaches the caller **unwrapped**. + ``get`` re-raises a :exc:`ValueError` that is already a + :exc:`~pcapkit.utilities.exceptions.BaseError` exactly as the override raised it, + and converts only :mod:`aenum`'s and :mod:`enum`'s own "no member carries this + value". Two things follow, and both are the point of the discrimination rather + than side effects: the override's **own message** survives to the caller instead + of being replaced by the base's, and the error is **logged once** rather than + twice, since :class:`~pcapkit.utilities.exceptions.BaseError` logs on + construction and re-wrapping would construct a second one. So an override should + say in its message what it rejected and why; that message is what the caller + sees. * **A** ``str`` **key never reaches it.** That path never calls ``cls(key)``, so it resolves only against already-populated lookup tables, where every value present is legal by construction. ``register`` does call it, after its duplicate check. @@ -361,9 +387,70 @@ Three things about it are easy to get wrong: Re-parenting the remaining helper enumerations onto :class:`~pcapkit.corekit.enum.EnumLookup` is **phase 2** of - `#877 `__ and has not happened - yet. Phase 1 is deliberately behaviour-preserving on its own, which is what let it - land while other work was still in flight on the files the re-parent touches. + `#877 `__, and it is **partly + done** rather than pending: the phase landed for 17 of the 24 non-registry + enumerations. **Seven are still outside the hierarchy**, measured by a runtime + walk over both the :mod:`enum` and :mod:`aenum` flavours: ``CommandType`` and + ``ConformanceRequirement`` in :mod:`pcapkit.const.ftp.command`, ``ESPStatus`` in + :mod:`pcapkit.protocols.internet.esp`, and all four + :mod:`pcapkit.protocols.internet.mh` helpers + (``FastBindingAcknowledgmentStatus``, ``IPv6AddressPrefixCode``, + ``LMAAddressCode``, ``LocalizedRoutingStatus``). Each sat in a file another pull + request held open while phase 2 ran, which is the whole reason the phase was split + in two: phase 1 is behaviour-preserving on its own, so it could land while work + was still in flight on the files a re-parent touches. Do not read the seven as a + ruling against re-parenting them -- they are the remainder of a phase, not an + exception to it. + +What a Failed Lookup Raises +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Two rules govern it, and they pull in opposite directions on purpose. The owner's +ruling, verbatim, on +`#923 `__: + + Either ``ValueError`` or ``KeyError``, that's depending on how stdlib's ``Enum`` + would raise on these circumstances. And we should raise one from + ``pcapkit.utilities.exceptions`` rather builtin exceptions. + +So the **provenance** is in-library and the **shape** is stdlib's: + +* :meth:`~pcapkit.corekit.enum.EnumLookup.get` raises + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` for a **name** miss and + :exc:`~pcapkit.utilities.exceptions.EnumValueError` for a **value** miss, both + from :mod:`pcapkit.utilities.exceptions` rather than from builtins. +* The split between the two is not taste. ``E['nosuch']`` raises :exc:`KeyError` + and ``E(999)`` raises :exc:`ValueError` on a stdlib :class:`~enum.Enum`, so a + miss by name is :exc:`KeyError`-derived here and a miss by value is + :exc:`ValueError`-derived, matching it. +* That is what keeps the ruling cheap to carry out: + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` derives :exc:`KeyError` and + :exc:`~pcapkit.utilities.exceptions.EnumValueError` derives :exc:`ValueError`, + so **only the provenance changed** -- every ``except KeyError`` and + ``except ValueError`` around a lookup keeps catching, in this tree and in a + caller's. + +Do not "improve" on the shape by making both misses report identically. Converting +one into the other is exactly what #923 retired, and it was retired in three places +at once: ``TransportProtocol.get`` and ``Criticality.get`` had each turned the +base's :exc:`KeyError` into a :exc:`ValueError`, and +:meth:`~pcapkit.protocols.internet.mh.FastBindingAcknowledgmentStatus.get` raised +:exc:`~pcapkit.utilities.exceptions.EnumValueError` for a name miss so that "the +two ways of getting it wrong reported identically". + +One asymmetry between the two is deliberate and is **not** visible from the +exception class: the **name** miss is raised quietly +(:class:`~pcapkit.utilities.exceptions.BaseError`'s ``quiet=True``, so nothing is +logged and :data:`sys.tracebacklimit` is left alone) while the **value** miss stays +loud. A name miss is in-library control flow at several call sites, and at +:meth:`~pcapkit.const.http.method.Method.get` it is part of a *successful* call -- +that override catches it in order to mint. A loud error there would put a +:data:`logging.CRITICAL` record on every such call and set +:data:`sys.tracebacklimit` to ``0`` process-wide, which is the +`#362 `__ defect ``quiet`` +exists for. So a ``get`` override that catches a name miss as control flow is +following the convention; one that catches a *value* miss that way is silencing a +logged error, and needs a reason. Case Sensitivity Is RFC-Directed ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -389,6 +476,22 @@ to make a lookup work -- which is what keeps ``R1_COUNTER = 129``, two IANA-registered HIP parameters differing only in case, both resolvable. +And a folding override carries **only** the fold. ``TransportProtocol.get`` is the +worked example: since +`#923 `__ it lowers ``key``, +forwards ``default`` verbatim and delegates to ``super().get()``, and that is all it +does. It used to convert the base's name-miss :exc:`KeyError` into a +:exc:`ValueError` as well, and #923's ruling retired that; the +`#836 `__ refusal to extend the +class at all is untouched by the retirement, since only the exception class moved. +``Criticality.get`` went further and no longer exists: conversion was the *only* +thing it added over the base, so once that went there was nothing left for an +override to hold, and the class inherits +:meth:`~pcapkit.corekit.enum.EnumLookup.get` unchanged. **An override that would +now be empty is deleted, not kept as a pass-through** -- a ``get`` that only calls +``super().get()`` reads as though it were doing something, and the next reader has +to diff it against the base to find out that it is not. + The Lenient Criterion, in Two Limbs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -503,7 +606,8 @@ That leaves the classes with something to decide: its ยง10.2 templates write the field upper case, the live CSV is lower case in all 14,536 rows (``tcp`` 6608, ``udp`` 6357, blank 1467, ``sctp`` 93, ``dccp`` 11, zero upper-case). - - **case-insensitive** -- ``get`` folds, and this is the owner's own example + - **case-insensitive** -- ``get`` folds, and this is the owner's own example. + Folding is now the *only* thing that override adds (#923) * - :class:`~pcapkit.const.reg.apptype.apptype.AppType` - -- - Moot: its ``get`` takes a port number and refuses a non-:class:`int` outright, @@ -527,13 +631,19 @@ That leaves the classes with something to decide: :mod:`~pcapkit.protocols.application.ngap` helper enumerations - IANA Mobility Header registries, 3GPP TS 38.413 - Their values are numeric codes, so the criterion is vacuous exactly as for the - :class:`int` tier above. Three of them -- ``Criticality``, - ``FastBindingAcknowledgmentStatus``, ``IPv6AddressPrefixCode`` -- do override - ``get``, but for signature reasons (no ``default``, and an - :class:`int`/:class:`str` dispatch) rather than for case: each does an exact - ``Cls[key]``. ``LMAAddressCode`` and ``LocalizedRoutingStatus`` carry no - ``get`` at all, so they have no string lookup to fold. Verified by reading all - five. + :class:`int` tier above. **Two** of them define a ``get`` of their own -- + ``FastBindingAcknowledgmentStatus`` and ``IPv6AddressPrefixCode`` -- for + signature reasons (no ``default``, and an :class:`int`/:class:`str` dispatch) + rather than for case: each does an exact ``Cls[key]``, and since #923 each + answers a name miss with + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` rather than + :exc:`~pcapkit.utilities.exceptions.EnumValueError`. ``LMAAddressCode`` and + ``LocalizedRoutingStatus`` carry no ``get`` at all, so they have no string + lookup to fold. ``Criticality`` had one when this audit was taken and no + longer does: #921 re-parented it onto + :class:`~pcapkit.corekit.enum.EnumLookup` and #923 retired the exception + conversion that was the override's only remaining job, so it now inherits + ``get`` unchanged. - **case-sensitive** -- conforms * - ``WireGuardKeyLabel`` - ``draft-tuexen-opsawg-pcapng`` @@ -567,7 +677,8 @@ wider than a case fix: these two case-insensitive. Nothing looks them up by string today, though: the crawler translates the CSV's lower-case letters to the upper-case member names at generation time, and neither class inherits - :class:`~pcapkit.corekit.enum.EnumLookup` yet. Re-parenting them is phase 2 of + :class:`~pcapkit.corekit.enum.EnumLookup` yet -- both are in the seven phase 2 has + not reached, per the note above. Re-parenting them is phase 2 of `#877 `__, which is where the question belongs. @@ -625,8 +736,10 @@ rather than changing it. What does survive is narrower and deliberate: a **declared-but-unassigned** ``str`` value resolves through ``cls(value)`` but not through ``get(value)``, because :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` returns it without -growing either lookup table. ``FEATCode.get('ZZ-NOT-REAL')`` raises :exc:`KeyError` -while ``FEATCode('ZZ-NOT-REAL')`` yields an unregistered member. Closing that gap would +growing either lookup table. ``FEATCode.get('ZZ-NOT-REAL')`` raises +:exc:`~pcapkit.utilities.exceptions.EnumKeyError` -- which *is* a :exc:`KeyError`, so +an ``except KeyError`` around it is unaffected -- while +``FEATCode('ZZ-NOT-REAL')`` yields an unregistered member. Closing that gap would mean calling ``cls(key)`` for a ``str`` value too, which reopens the minting hazard above -- so the asymmetry is intended, and ``get``'s own docstring carries the full reasoning. @@ -637,3 +750,193 @@ reasoning. generated registry belongs in the crawler or in :mod:`pcapkit.vendor.default`'s template, never in the generated file alone -- the next regeneration would discard it. + +.. _extension-header-subclassing: + +Which bases an IPv6 extension header names +------------------------------------------ + +Every IPv6 extension header in this package subclasses +:class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`. Some name a **second** base +as well, and which ones do is a ruling rather than an accident. The owner's words, +on `#924 `__: + + I think on subclassing, we might wanna keep this convention: if the IPv6 + extension header is only usable as an extension header, then it only inherit + from ``IPv6_Ext``, like ``IPv6_Frag``; but if it is useable as a standalone + protocol itself, then it herit from both ``IPv6_Ext`` and ``Internet`` (or + ``IPsec``), like ``ESP``. + +The family as it stands: + +.. list-table:: + :header-rows: 1 + :widths: 34 30 36 + + * - Header + - Bases + - Classification + * - :class:`~pcapkit.protocols.internet.hopopt.HOPOPT`, + :class:`~pcapkit.protocols.internet.ipv6_route.IPv6_Route`, + :class:`~pcapkit.protocols.internet.ipv6_frag.IPv6_Frag`, + :class:`~pcapkit.protocols.internet.ipv6_opts.IPv6_Opts`, + :class:`~pcapkit.protocols.internet.mh.MH` + - ``IPv6_Ext`` + - extension-header only + * - :class:`~pcapkit.protocols.internet.ah.AH`, + :class:`~pcapkit.protocols.internet.esp.ESP` + - ``IPsec``, ``IPv6_Ext`` + - **also standalone** + * - :class:`~pcapkit.protocols.internet.hip.HIP` + - ``IPv6_Ext``, ``Internet`` + - **also standalone** + +:class:`~pcapkit.protocols.internet.ipsec.IPsec` is itself an +:class:`~pcapkit.protocols.internet.internet.Internet` subclass, which is the +parenthetical *"(or* ``IPsec``\ *)"* in the ruling: naming it satisfies the +convention, and it is the right second base for a header whose standalone form is an +IPsec one. + +The code cannot be used as evidence +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +**This is the part a future reader will get wrong, so it is stated before the +criterion itself.** The obvious way to decide whether a header is "usable as a +standalone protocol" is to ask what the library's own dispatch already allows. That +answer is useless, and measurably so: + +.. code-block:: pycon + + >>> from pcapkit.protocols.internet.ipv4 import IPv4 + >>> from pcapkit.protocols.internet.internet import Internet + >>> IPv4.__proto__ is Internet.__proto__ + True + +The protocol-number registry is **one shared object**, so *every* extension header +is reachable as an IPv4 payload in this library -- +:class:`~pcapkit.protocols.internet.hopopt.HOPOPT` and +:class:`~pcapkit.protocols.internet.ipv6_frag.IPv6_Frag` included. Registry +membership therefore says nothing at all about standalone-ness, and a classification +derived from it would make all eight headers standalone. + +Nor does IANA's *IPv6 Extension Header Types* registry discriminate: it lists all +eight of the implemented headers, plus ``Shim6`` (140) and 253/254. Being *in* that +registry is what makes something an extension header; it is not evidence about +whether the same header is also a protocol in its own right. + +The operative test is what the RFCs say +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +So the census is read out of the specifications, on the owner's instruction: + + So my suggestion is to read through the RFCs to figure out any of the defined + IPv6 extension headers are extension header only or standalone protocol as well. + Then we can decide if they should inherit only ``IPv6_Ext`` or additional bases. + +And the limb that decides is **whether a primary source shows the header carried +directly as an IPv4 payload**: + +* :class:`~pcapkit.protocols.internet.ah.AH` -- :rfc:`4302#section-3.1.1`, *"In the + context of IPv4, this calls for placing AH after the IP header"*, with a + before-and-after IPv4 diagram. +* :class:`~pcapkit.protocols.internet.esp.ESP` -- :rfc:`4303#section-3.1.1`, the + same sentence and the same diagram for ESP. +* :class:`~pcapkit.protocols.internet.hip.HIP` -- :rfc:`7401#appendix-C.2`, + *"IPv4 HIP Packet (I1 Packet)"*, whose worked checksum is over an IPv4 header + carrying ``Next Header: 139``. :rfc:`7401#section-5.1` also calls the HIP header + *"logically an IPv6 extension header"*, so HIP is genuinely both. + +:class:`~pcapkit.protocols.internet.mh.MH` is the instructive failure, because it +**is** a protocol in its own right and still does not qualify: +:rfc:`6275#section-6.1.1` defines its checksum over a pseudo-header of *"IPv6 header +fields"* whose addresses are *"the addresses that appear in the Source and +Destination Address fields in the IPv6 packet carrying the Mobility Header"* -- there +is no IPv4 variant of that computation -- and the IPv4 equivalent function is not +protocol 135 at all, since :rfc:`5944` carries Mobile IPv4 over UDP port 434. +``Shim6`` (140) has the same shape and the same verdict; this package has never had a +parser class for it, so nothing implements the classification, but a future one +inherits :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` alone. + +.. important:: + + Own-protocolhood on its own is **not** sufficient, and MH is the case that + settles it: the alternative reading -- that a protocol in its own right qualifies + whether or not it can appear under IPv4 -- was put to the owner explicitly on + `#924 `__ and not taken, so MH + and ``Shim6`` stay extension-only. A header that is a protocol in its own right + but structurally cannot be an IPv4 payload names ``IPv6_Ext`` alone. + +The declaration is what carries the classification +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Name the second base **explicitly**, even though it is already in the MRO. +:class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` derives +:class:`~pcapkit.protocols.internet.internet.Internet`, so every one of the eight +reaches ``Internet`` transitively and an ``__mro__`` check cannot tell the two groups +apart. The declaration is the only place the classification exists, which is why +``tests/protocols/internet/test_ipv6_ext_unit.py`` pins it against ``__bases__``: + +.. code-block:: python + + STANDALONE_MEMBERS = frozenset({'AH', 'ESP', 'HIP'}) + +Add a header to that set in the same change that adds its second base, and keep the +RFC ground in the ``#:`` comment beside it. The test walks +``IPv6_Ext.__subclasses__()`` rather than a hard-coded list, so a ninth header is +held to the convention whether or not anyone remembers this page. + +The base is named ``IPv6_Ext``, and nothing else +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The class arrived as ``IPv6_GenericExt``, a fallback parser for an unrecognised +extension header (`#891 `__), and +`#917 `__ merged that role with +the shared-base role into one class under the shorter name. No compatibility alias +was left behind, and that was deliberate. The owner's ruling: + + No more ``IPv6_GenericExt`` name. Its an intermediate state and never released. + +The reasoning is what makes it safe rather than merely decided: the old name existed +on ``main`` from ``b3551cb63`` to ``93cf940b3`` -- under four hours on one day, and +after the most recent release tag -- so it appears in **no** release, and the break +has no callers to inconvenience. Do not reintroduce it as an alias, and do not cite +it in prose as a former public name; it was never one. + +ESP is an extension header, and still cannot short-circuit +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Two facts about :class:`~pcapkit.protocols.internet.esp.ESP` coexist, and each is +routinely mistaken for a refutation of the other. + +**It is an extension header.** IANA's *IPv6 Extension Header Types* registry lists +protocol number **50**, and this package's +:class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader` agrees (``ESP = 50``). +:rfc:`8200#section-4.5` appears to say otherwise -- *"the Encapsulating Security +Payload (ESP) is not considered an extension header"* -- but that sentence opens +*"For this purpose,"*, scoping it to the fragmentation discussion it sits in, and the +sentence after it lists ESP among *"examples of upper-layer headers"*. The library +follows the registry, on the owner's ruling for +`#895 `__, which is why ESP +carries the same extension-mode contract as its siblings. + +**And it terminates the chain walk.** :rfc:`4303` places ESP's Next Header byte +inside the *encrypted* trailer, so with no key material there is nothing to continue +on: ESP's own data model reports ``next`` as :obj:`None`, and +:meth:`IPv6._decode_next_layer ` +ends the ordinary way one iteration later. So ESP is absent from +``IPv6.__generic_ext_codes__`` -- not because it lacks a parser, which it has, but +because it cannot hand the walk a successor. + +The full reasoning, including how ESP's terminal case differs from 253/254's, is +written where the code is and is deliberately not restated at length here: see the +``#:`` comment on ``IPv6.__generic_ext_codes__`` +(:file:`pcapkit/protocols/internet/ipv6.py`) and the module docstring of +:mod:`pcapkit.protocols.internet.ipv6_ext`. + +.. caution:: + + The two facts have to be kept apart when reading any of this. "ESP is not an + extension header" (wrong, and :rfc:`8200#section-4.5` quoted out of scope) is a + different claim from "ESP cannot be walked past" (right, and about + :rfc:`4303`'s wire format). Collapsing them is how ESP ends up either dropped + from the extension-header contract or wrongly added to the generic-dispatch set. diff --git a/docs/source/pcapkit/utilities/exceptions.rst b/docs/source/pcapkit/utilities/exceptions.rst index ce485322b..9b21dedbc 100644 --- a/docs/source/pcapkit/utilities/exceptions.rst +++ b/docs/source/pcapkit/utilities/exceptions.rst @@ -200,6 +200,10 @@ It is still an ordinary exception carrying its message, so ``except`` clauses an :no-members: :show-inheritance: +.. autoexception:: pcapkit.utilities.exceptions.EnumValueError + :no-members: + :show-inheritance: + .. autoexception:: pcapkit.utilities.exceptions.FieldValueError :no-members: :show-inheritance: diff --git a/tests/const/test_const_ftp_featcode_case_903_unit.py b/tests/const/test_const_ftp_featcode_case_903_unit.py index cdb6d6409..297be0004 100644 --- a/tests/const/test_const_ftp_featcode_case_903_unit.py +++ b/tests/const/test_const_ftp_featcode_case_903_unit.py @@ -61,7 +61,7 @@ **What the fix deliberately does not do.** It does not fold the stored members, and it does not rename one. Every member keeps the registrar's own -casing, per the ruling on the *Registry Conventions* page -- *"enum +casing, per the ruling on the *House Conventions* page -- *"enum should honour and keep their original writings as in the registrars"* -- so ``FEATCode.get('BASE').name`` is still ``'base'`` and still says *placeholder*. Nor does it fold the *value* a lookup resolves to, which is @@ -133,7 +133,7 @@ def test_the_value_form_folds_too(self) -> 'None': ``FEATCode.base`` is the member whose value is ``''`` and whose name is ``'base'`` -- the shape - the *Registry Conventions* page uses to demonstrate the base's + the *House Conventions* page uses to demonstrate the base's name-misses-then-value-matches fall-through. Folding has to reach the value side as well, or a caller holding the registry's own value string in the RFC's recommended casing still fails. @@ -208,7 +208,7 @@ def test_a_folded_name_never_shadows_another_members_value(self) -> 'None': self.assertIs(by_folded_name[folded_value], member) def test_an_unknown_code_still_raises_rather_than_minting(self) -> 'None': - """The asymmetry the *Registry Conventions* page records survives. + """The asymmetry the *House Conventions* page records survives. ``get`` never calls ``cls(key)`` for a ``str``, so a key matching no member -- in any casing -- raises instead of growing the registry, diff --git a/tests/project/test_conventions_doc_claims.py b/tests/project/test_conventions_doc_claims.py new file mode 100644 index 000000000..9fa31a8f7 --- /dev/null +++ b/tests/project/test_conventions_doc_claims.py @@ -0,0 +1,450 @@ +# -*- coding: utf-8 -*- +"""Claims on the *House Conventions* page that the tree can be asked about. + +:file:`docs/source/contributing/conventions.rst` records design rulings, and most of +what it records is reasoning -- which no test can check. Some of it is not: a count of +classes, a per-class classification, a retired name, an exception type. Those are the +parts that rot silently, because Sphinx builds without ``-W`` and without ``nitpicky``, +so a page whose every factual claim has gone stale still renders and CI still passes. + +GitHub issue #918 harvested the settled rulings onto that page, and this file pins the +checkable ones: + +* :class:`ConventionAnchorTests` -- the four ``.. _label:`` anchors the page carries. + Three of them are cross-referenced from :mod:`pcapkit` docstrings and from + ``tests/corekit/test_sentinel_exports_unit.py``, which slices the file *by* two of + them, so a later split of the page into one file per section has to keep every anchor + resolving. This is the cheap guard that a split cannot orphan one by accident. +* :class:`PhaseTwoRemainderTests` -- the three counts the page states about + `#877 `__'s phase 2, measured + rather than remembered. The page said the phase *"has not happened yet"* for as long + as it did because nothing contradicted it once #921 landed. +* :class:`ExtensionHeaderClassificationTests` -- the page's bases-per-header table + against the declarations themselves. ``tests/protocols/internet/test_ipv6_ext_unit.py`` + already pins the *code* to the ruling; nothing pinned the *page* to the code, which is + the half that goes stale when a ninth header lands. +* :class:`RetiredNameTests` -- *"No more ``IPv6_GenericExt`` name."* A ruling that a name + must not exist is exactly the kind a later change reintroduces without noticing. +* :class:`FailedLookupExceptionTests` -- the worked example the page gives for a name + miss, which named :exc:`KeyError` until #918 and now names + :exc:`~pcapkit.utilities.exceptions.EnumKeyError`. + +Deliberately **not** checked here: whether the page's cross-references resolve. That is +a property of the built inventory rather than of the source, for the reason +:file:`tests/project/test_documentation_claims.py` gives at length, and the honest check +is the rendered HTML. The measured result is recorded in the pull request instead. + +""" + +from __future__ import annotations + +import enum +import importlib +import pathlib +import pkgutil +import re +import unittest + +import aenum + +ROOT = pathlib.Path(__file__).resolve().parents[2] + +#: The page itself. Hard-coded rather than discovered, because the path is *also* what +#: ``tests/corekit/test_sentinel_exports_unit.py`` hard-codes -- so if the page moves, +#: both files have to be updated together and a test that found it either way would +#: hide half of that. +CONVENTIONS = ROOT / 'docs' / 'source' / 'contributing' / 'conventions.rst' + +#: Every ``.. _label:`` the page is cross-referenced by, and what each is for. The first +#: three predate #918; ``extension-header-subclassing`` arrived with it. +ANCHORS = ( + 'mint-criterion', + 'sentinel-convention', + 'registry-protocol', + 'extension-header-subclassing', +) + +#: Number words as the page spells them, so a count can be read back out of the prose. +#: The page states its figures in words rather than digits, which is house style there. +NUMBER_WORDS = { + 'one': 1, 'two': 2, 'three': 3, 'four': 4, 'five': 5, 'six': 6, 'seven': 7, + 'eight': 8, 'nine': 9, 'ten': 10, 'eleven': 11, 'twelve': 12, +} + + +def _page() -> 'str': + """The page's text. + + Raises: + AssertionError: If the page is not where every reference to it says it is. + + """ + if not CONVENTIONS.is_file(): # pragma: no cover + raise AssertionError( + f'conventions.rst not found at {CONVENTIONS}; pcapkit/corekit/sentinels.py ' + 'and tests/corekit/test_sentinel_exports_unit.py both name this path' + ) + return CONVENTIONS.read_text(encoding='utf-8') + + +def _section(anchor: 'str') -> 'str': + """The page text from ``anchor`` up to the next anchor, or to the end. + + Sliced by the anchors rather than by line number, following + ``tests/corekit/test_sentinel_exports_unit.py``, so an edit elsewhere on the page + cannot silently make an assertion read the wrong section. + + A missing anchor fails here with a message naming it, rather than with + :meth:`str.index`'s bare ``substring not found`` -- and the *other* anchors are + looked up with :meth:`str.find` for the same reason, so one missing label does not + make every slice on the page report the wrong thing. + + Args: + anchor: The label to slice from, without the ``.. _`` and ``:``. + + Returns: + The section's text. + + Raises: + AssertionError: If ``anchor`` is not on the page at all. + + """ + text = _page() + start = text.find(f'.. _{anchor}:') + if start < 0: + raise AssertionError(f'anchor .. _{anchor}: is not on {CONVENTIONS.name}; ' + 'every :ref: pointing at it is now plain text') + following = [found for other in ANCHORS + if (found := text.find(f'.. _{other}:')) > start] + return text[start:min(following)] if following else text[start:] + + +def _every_enumeration() -> 'dict[type, str]': + """Every enumeration :mod:`pcapkit` defines, nested ones included. + + Both the :mod:`enum` and :mod:`aenum` flavours, because the tree uses both and a + walk over one of them silently under-reports. Nested classes are walked too: the + seven ``Flags`` enumerations under + :mod:`pcapkit.protocols.schema.application.httpv2` are all nested, and they are + what makes the difference between 17 non-registry enumerations and 24. + + :mod:`pcapkit.vendor` is skipped. Its modules are crawlers whose classes are + :class:`~pcapkit.vendor.default.Vendor` subclasses named after registries rather + than enumerations themselves, so nothing there is in scope and importing them is + only cost. + + Returns: + Each enumeration mapped to its dotted qualified name. + + """ + import pcapkit + + for module in pkgutil.walk_packages(pcapkit.__path__, 'pcapkit.'): + if module.name.startswith('pcapkit.vendor'): + continue + # Not guarded: every module under pcapkit imports cleanly, and swallowing an + # ImportError here would let this walk go quietly partial -- which is the one + # failure mode that would make every count below pass vacuously. + importlib.import_module(module.name) + + found = {} # type: dict[type, str] + visited = set() # type: set[type] + + def walk(container: 'type') -> 'None': + """Recurse through ``container``'s own attributes. + + Into **every** nested class, not only the enumerations: the seven ``Flags`` + are nested inside schema classes, which are not enumerations themselves, so a + recursion that only followed enumerations would never reach them -- and would + under-report the non-registry population by exactly those seven. + + """ + if container in visited: + return + visited.add(container) + for value in vars(container).values(): + if not isinstance(value, type): + continue + if not getattr(value, '__module__', '').startswith('pcapkit'): + continue + if issubclass(value, (enum.Enum, aenum.Enum)): + found.setdefault(value, f'{value.__module__}.{value.__qualname__}') + walk(value) + + import sys + + for name, module in list(sys.modules.items()): + if not name.startswith('pcapkit') or name.startswith('pcapkit.vendor'): + continue + for value in vars(module).values(): + if not isinstance(value, type): + continue + if not getattr(value, '__module__', '').startswith('pcapkit'): + continue + if issubclass(value, (enum.Enum, aenum.Enum)): + found.setdefault(value, f'{value.__module__}.{value.__qualname__}') + walk(value) + return found + + +class ConventionAnchorTests(unittest.TestCase): + """The labels other files cross-reference, all in the one page.""" + + def test_every_cross_referenced_anchor_is_present(self) -> 'None': + """A missing anchor is a dead ``:ref:`` that renders as plain text. + + Sphinx is run here without ``-W`` and without ``nitpicky``, so an unresolved + reference is not a build failure -- it is a word that used to be a link. This + is the assertion a split of the page has to keep passing. + + """ + text = _page() + for anchor in ANCHORS: + with self.subTest(anchor=anchor): + # ``assertTrue`` rather than ``assertIn``: the latter dumps the whole + # 900-line page into the failure and buries the one line that says + # which anchor went missing. + self.assertTrue(f'.. _{anchor}:' in text, + f'.. _{anchor}: is no longer on {CONVENTIONS.name}') + + def test_the_sentinel_slice_markers_stay_in_one_file_and_in_order(self) -> 'None': + """``test_sentinel_exports_unit`` slices between two of the anchors. + + It reads ``text.index('.. _sentinel-convention:')`` through + ``text.index('.. _registry-protocol:', start)``, so splitting those two + sections into separate files breaks it -- not with a wrong answer, but with a + :exc:`ValueError` from :meth:`str.index`. Stated here as well so the + constraint is discoverable from the page's own tests. + + """ + text = _page() + self.assertLess(text.index('.. _sentinel-convention:'), + text.index('.. _registry-protocol:')) + + +class PhaseTwoRemainderTests(unittest.TestCase): + """The page's counts for GitHub issue #877's phase 2, measured.""" + + def setUp(self) -> 'None': + from pcapkit.corekit.enum import EnumLookup, EnumRegistry + + self.enumerations = _every_enumeration() + self.non_registry = {cls: name for cls, name in self.enumerations.items() + if not issubclass(cls, EnumRegistry)} + self.outside = {cls: name for cls, name in self.non_registry.items() + if not issubclass(cls, EnumLookup)} + # Whitespace-normalised, because the page wraps its prose at 88 columns and a + # sentence this reads a figure out of is routinely split across lines. + self.note = ' '.join(_section('registry-protocol').split()) + + def test_the_walk_found_something_to_count(self) -> 'None': + """Guards every count below from passing on an empty discovery.""" + self.assertGreater(len(self.enumerations), 100, + 'the enumeration walk collapsed; the counts below would ' + 'pass vacuously') + + def test_the_page_states_the_measured_number_outside_the_hierarchy(self) -> 'None': + """*"Seven are still outside the hierarchy"* -- against a runtime walk.""" + stated = re.search(r'\*\*(\w+) are still outside the hierarchy\*\*', self.note) + self.assertIsNotNone(stated, 'the page no longer states how many enumerations ' + 'are outside EnumLookup; the wording this test ' + 'reads has changed') + assert stated is not None # for type checkers; asserted above + self.assertEqual(NUMBER_WORDS[stated.group(1).lower()], len(self.outside), + f'the page says {stated.group(1)!r} but the tree has ' + f'{len(self.outside)}: {sorted(self.outside.values())}') + + def test_the_page_names_every_enumeration_outside_the_hierarchy(self) -> 'None': + """The count alone would pass on a wrong list of the right length.""" + for name in sorted(self.outside.values()): + with self.subTest(enumeration=name): + self.assertIn(name.rsplit('.', maxsplit=1)[-1], self.note) + + def test_the_page_states_the_measured_phase_two_progress(self) -> 'None': + """*"landed for 17 of the 24 non-registry enumerations"*, both figures.""" + stated = re.search(r'landed for (\d+) of the (\d+) non-registry enumerations', + self.note) + self.assertIsNotNone(stated, 'the page no longer states phase 2 progress in ' + 'the shape this test reads') + assert stated is not None # for type checkers; asserted above + done, total = int(stated.group(1)), int(stated.group(2)) + self.assertEqual(total, len(self.non_registry), + 'the page\'s non-registry enumeration count is stale') + self.assertEqual(done, len(self.non_registry) - len(self.outside), + 'the page\'s re-parented count is stale') + + +class ExtensionHeaderClassificationTests(unittest.TestCase): + """The page's bases-per-header table against the declarations themselves.""" + + def setUp(self) -> 'None': + import pcapkit.protocols.internet # noqa: F401 # populates __subclasses__ + + from pcapkit.protocols.internet.internet import Internet + from pcapkit.protocols.internet.ipv6_ext import IPv6_Ext + + self.internet = Internet + self.ipv6_ext = IPv6_Ext + self.family = {} # type: dict[str, type] + stack = list(IPv6_Ext.__subclasses__()) + while stack: + klass = stack.pop() + if klass.__qualname__ in self.family: + continue + self.family[klass.__qualname__] = klass + stack.extend(klass.__subclasses__()) + self.rows = self._table_rows() + + @staticmethod + def _table_rows() -> 'list[list[str]]': + """The classification table, one list of cells per row, header row included. + + Parsed from the ``.. list-table::`` markup rather than from a rendered build, + so this runs without the docs toolchain. Cell text is joined on whitespace, so + reflowing a cell across lines does not change what is asserted. + + Returns: + One list of cell strings per row. + + """ + section = _section('extension-header-subclassing') + lines = section[section.index('.. list-table::'):].splitlines() + + rows = [] # type: list[list[str]] + for line in lines[1:]: + if line.startswith(' * - '): + rows.append([line[len(' * - '):]]) + elif line.startswith(' - ') and rows: + rows[-1].append(line[len(' - '):]) + elif line.strip() and line.startswith(' ') and rows: + rows[-1][-1] += ' ' + line.strip() + elif line.strip() and not line.startswith(' '): + break # back at column 0: the table is over + return [[' '.join(cell.split()) for cell in row] for row in rows] + + @staticmethod + def _named(cell: 'str') -> 'tuple[str, ...]': + """Every class the cell names, whether as a ``:class:`` role or a literal.""" + roles = re.findall(r':class:`~[\w.]*\.(\w+)`', cell) + return tuple(roles) if roles else tuple(re.findall(r'``(\w+)``', cell)) + + def test_the_table_was_parsed(self) -> 'None': + """Guards the assertions below from passing on an unparsed table.""" + self.assertTrue(self.rows, 'the classification table did not parse; the page ' + 'markup this test reads has changed') + self.assertEqual(self.rows[0], ['Header', 'Bases', 'Classification'], + 'the table header changed, so the column order the ' + 'assertions below assume may no longer hold') + + def test_the_table_covers_the_whole_family_and_nothing_else(self) -> 'None': + """A ninth extension header has to reach the page, not only the code.""" + listed = {name for row in self.rows[1:] for name in self._named(row[0])} + self.assertEqual(listed, set(self.family), + 'the page and IPv6_Ext.__subclasses__() disagree about which ' + 'headers exist') + + def test_the_table_records_the_declared_bases_in_order(self) -> 'None': + """``__bases__``, not ``__mro__``. + + Every member reaches + :class:`~pcapkit.protocols.internet.internet.Internet` transitively through + :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`, so what the convention + encodes is the declaration. Asserting the cell against ``__bases__`` is + therefore asserting the thing the ruling is about. + + """ + for row in self.rows[1:]: + documented = self._named(row[1]) + for header in self._named(row[0]): + with self.subTest(header=header): + actual = tuple(base.__name__ + for base in self.family[header].__bases__) + self.assertEqual(actual, documented) + + def test_the_table_agrees_with_the_standalone_classification(self) -> 'None': + """*"also standalone"* on the page means a second ``Internet``-derived base.""" + for row in self.rows[1:]: + standalone = 'also standalone' in row[2] + for header in self._named(row[0]): + with self.subTest(header=header): + named = {base for base in self.family[header].__bases__ + if base is not self.ipv6_ext + and issubclass(base, self.internet)} + self.assertEqual(bool(named), standalone) + + def test_the_page_agrees_with_the_test_that_pins_the_code(self) -> 'None': + """The page and ``STANDALONE_MEMBERS`` are two records of one ruling. + + Read out of the test module rather than re-derived, so the two cannot drift + apart in the direction where the code changes and only one record follows. + + """ + from tests.protocols.internet.test_ipv6_ext_unit import ( + IPv6ExtSharedBaseContractTests) + + documented = {name for row in self.rows[1:] if 'also standalone' in row[2] + for name in self._named(row[0])} + self.assertEqual(documented, + set(IPv6ExtSharedBaseContractTests.STANDALONE_MEMBERS)) + + +class RetiredNameTests(unittest.TestCase): + """*"No more* ``IPv6_GenericExt`` *name. Its an intermediate state and never + released."*""" + + def test_the_retired_base_name_is_absent_from_the_package(self) -> 'None': + """A ruling that a name must not exist needs a test, or it comes back. + + Checked over the package source rather than by import, because the failure + this guards against is a *reintroduced alias* -- which would import perfectly + well and satisfy any behavioural assertion. + + """ + offenders = [str(path.relative_to(ROOT)) + for path in sorted((ROOT / 'pcapkit').rglob('*.py')) + if 'IPv6_GenericExt' in path.read_text(encoding='utf-8')] + self.assertEqual(offenders, [], + 'the retired name is back; the ruling on GitHub pull request ' + '#924 is that it was an intermediate state and never released') + + def test_the_shared_base_still_carries_the_name_the_ruling_left(self) -> 'None': + """The other half: the rename landed, rather than the name simply going.""" + from pcapkit.protocols.internet.ipv6_ext import IPv6_Ext + + self.assertEqual(IPv6_Ext.__name__, 'IPv6_Ext') + + +class FailedLookupExceptionTests(unittest.TestCase): + """The worked example the page gives for a lookup that does not resolve.""" + + def test_the_page_names_the_exception_the_code_actually_raises(self) -> 'None': + """It said :exc:`KeyError` until #918, which was true but no longer specific. + + ``EnumKeyError`` is what GitHub issue #923 made ``get`` raise, and it *is* a + :exc:`KeyError` -- which is why the stale wording never failed anything. + + """ + from pcapkit.const.ftp.command import FEATCode + from pcapkit.utilities.exceptions import EnumKeyError + + with self.assertRaises(EnumKeyError) as caught: + FEATCode.get('ZZ-NOT-REAL') + self.assertIsInstance(caught.exception, KeyError) + + section = ' '.join(_section('registry-protocol').split()) + at = section.index("``FEATCode.get('ZZ-NOT-REAL')``") + self.assertIn('EnumKeyError', section[at:at + 300], + 'the page still describes the name miss without naming ' + 'EnumKeyError') + + def test_a_declared_but_unassigned_value_still_resolves_through_the_constructor( + self) -> 'None': + """The other half of the asymmetry the page records, so it stays a pair.""" + from pcapkit.const.ftp.command import FEATCode + + self.assertNotIn('ZZ-NOT-REAL', FEATCode._member_map_) + self.assertEqual(FEATCode('ZZ-NOT-REAL').value, 'ZZ-NOT-REAL') + + +if __name__ == '__main__': + unittest.main()