From 9f86eba663735529275055e243860998441eb1c3 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 30 Sep 2026 01:11:14 -0400 Subject: [PATCH] docs(contributing): split conventions.rst into one file per anchor (#918) Part 2 of #918. The 966-line docs/source/contributing/conventions.rst carried four unrelated rulings on one page; split into docs/source/contributing/conventions/, one file per `.. _label:` anchor, plus index.rst carrying the toctree and the `.. important::` preamble. Pure move -- every split file is byte-identical to its slice of the original. - Anchors are global in Sphinx, so no `:ref:` needed touching; only the one `:doc:` path -- docs/source/index.rst's toctree entry -- named the retired bare document and now points at conventions/index. - test_sentinel_exports_unit.py's `_sentinel_section` used to slice between `.. _sentinel-convention:` and `.. _registry-protocol:` in one shared file, exactly what #930 flagged as the split's blocker. Now reads sentinel-convention.rst whole. - test_conventions_doc_claims.py rewritten for the split: reads each anchor's own file, and gains tests pinning the split's own shape (every anchor lives in exactly one file, the index's toctree lists all four, the top-level index points at the new page). - Corrected ten prose citations of the old single-file path, across pcapkit/corekit/sentinels.py and eight test files, to name the file each now actually lives in. - Cross-review round: the index's `.. important::` preamble carried over unedited, so it still spoke as a single page -- "this page records" every ruling, and told a future contributor to write a new one "onto this page". Both went false once the index stopped holding any ruling of its own. Reworded to speak as a hub (the standing #918 instruction now points at "the page that covers it"), same voice and both owner quotes kept, and pinned with a test that bans the self-referential "this page" from the index and checks the corrected phrase landed. Build: nitpicky sphinx-build warning set unchanged (1287, byte- identical to 9ea0d6a5a once build-order nondeterminism in ambiguous xref candidate lists is normalised); isort clean; pylint 10.00/10 on sentinels.py (unchanged). The four split pages remain byte-identical to their slices of 9ea0d6a5a's original conventions.rst. --- .../extension-header-subclassing.rst | 189 +++++++ .../source/contributing/conventions/index.rst | 31 ++ .../conventions/mint-criterion.rst | 117 ++++ .../registry-protocol.rst} | 498 ------------------ .../conventions/sentinel-convention.rst | 168 ++++++ docs/source/index.rst | 2 +- pcapkit/corekit/sentinels.py | 8 +- tests/corekit/test_enum_lookup_base_unit.py | 6 +- .../test_fields_numbers_unassigned_enum.py | 3 +- tests/corekit/test_sentinel_exports_unit.py | 46 +- tests/project/test_conventions_doc_claims.py | 314 ++++++++--- .../test_ftp_featcode_doc_page_934_unit.py | 17 +- .../test_sentinels_doc_page_934_unit.py | 13 +- tests/protocols/internet/test_ah_unit.py | 10 +- tests/protocols/internet/test_esp_unit.py | 10 +- tests/protocols/misc/test_pcapng_unit.py | 6 +- 16 files changed, 802 insertions(+), 636 deletions(-) create mode 100644 docs/source/contributing/conventions/extension-header-subclassing.rst create mode 100644 docs/source/contributing/conventions/index.rst create mode 100644 docs/source/contributing/conventions/mint-criterion.rst rename docs/source/contributing/{conventions.rst => conventions/registry-protocol.rst} (51%) create mode 100644 docs/source/contributing/conventions/sentinel-convention.rst diff --git a/docs/source/contributing/conventions/extension-header-subclassing.rst b/docs/source/contributing/conventions/extension-header-subclassing.rst new file mode 100644 index 000000000..766f4b192 --- /dev/null +++ b/docs/source/contributing/conventions/extension-header-subclassing.rst @@ -0,0 +1,189 @@ +.. _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/contributing/conventions/index.rst b/docs/source/contributing/conventions/index.rst new file mode 100644 index 000000000..c2c76f035 --- /dev/null +++ b/docs/source/contributing/conventions/index.rst @@ -0,0 +1,31 @@ +House Conventions +================= + +.. important:: + + The pages below record **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* + until `#918 `__ widened + it, then split 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 + the page that covers it in the same change that implements it, rather than + 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."* + +.. toctree:: + :maxdepth: 1 + + mint-criterion + sentinel-convention + registry-protocol + extension-header-subclassing diff --git a/docs/source/contributing/conventions/mint-criterion.rst b/docs/source/contributing/conventions/mint-criterion.rst new file mode 100644 index 000000000..1cfc10cd0 --- /dev/null +++ b/docs/source/contributing/conventions/mint-criterion.rst @@ -0,0 +1,117 @@ +.. _mint-criterion: + +When an unrecognised value may mint a member +-------------------------------------------- + +Every registry under :mod:`pcapkit.const` defines ``_missing_``, which decides what +happens when a value has no member. There are two possible behaviours, and which one +a given range gets is a **design decision, not a style preference**: + +``extend_enum(cls, name, value)`` -- *mint* + Creates a real, permanent member on the class. It is installed in + ``_member_map_`` and ``_value2member_map_``, so it is visible to iteration, + lookup and ``__members__`` from then on, for the life of the process. + +:meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` -- *unmint* + Returns a member-like object for the value **without** installing it. The + registry does not grow, and a second lookup of the same value is indistinguishable + from the first. + +The criterion +~~~~~~~~~~~~~ + +The test, in the maintainer's words: + + Is this considered as the final concrete assigned name (**mint**), or just a + notation for the readers (**unmint**)? + +Settled on `#847 `__ and confirmed +as "a core concept of the ruling" on +`#775 `__. + +So the question to ask of a range is **what the upstream registry actually did**, not +what the generated code happens to look like: + +* The source assigns a **real, specific name** to those codes -- minting records + something the registry genuinely says. **Mint.** +* The source says only that the codes are spoken for, without naming them -- + ``Unassigned``, ``Reserved``, ``Reserved for Private Use``, + ``Reserved for Experimental Use``, ``Deprecated``, ``Dynamically Assigned`` + and ``Statically Assigned``. These are written for a human reading the + table. Minting them **manufactures a name nobody + assigned**, and the value will get its real name if and when something assigns + it. **Unmint.** + +Worked examples +~~~~~~~~~~~~~~~ + +*Unmint.* :mod:`pcapkit.const.ipx.socket`'s ``Dynamically Assigned``, +``Dynamically Assigned Socket Numbers``, ``Statically Assigned Socket Numbers`` and +``Experimental`` ranges. Each names **how the socket will be allocated**, not what +occupies it; the real name arrives with the allocation. + +*Mint.* :mod:`pcapkit.const.reg.ethertype`'s company names -- ``Xyplex``, +``Datability``, ``Qualcomm``, ``Motorola`` and forty-two others, 46 names across 50 +range blocks, since four of them hold two blocks each -- and, in the same file's +neighbour, :mod:`pcapkit.const.ipx.socket`'s ``Registered by Xerox``. A company name +is the assignment, for the reason in the next section. + +Two further ranges in that file mint without being company names at all: +``IEEE802.3 Length Field``, which names a field in a standard, and +``Berkeley Trailer encap/IP``, which names an encapsulation. Both are outside the 46, +and neither mints for the reason the next section gives. + +.. note:: + + Those two groups look alike and the line between them is **not** range-versus-single + code. ``Registered by Xerox`` covers a range and still mints, because it names *who + registered the socket*. ``Dynamically Assigned`` also covers a range and does not, + because it names only the *mechanism* by which some future party will take it. Ask + what the label tells you: a party, or a procedure. + +Why the company names mint +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ethertype case looks like an exception to the rule and is not. The maintainer's +reasoning, settled on `#775 `__ after +being raised on `#847 `__: + + Proprietary protocols won't have public names so company names serve this purpose. + +So the company name is not a note *about* the code -- it is the best name that will ever +exist *for* it, which makes it the final concrete assigned name under the test above. +That is also why ``DEC Unassigned`` goes the other way despite carrying the same +attribution: there the company holds the block and assigned nothing, so the notation is +"Unassigned" and the attribution is incidental. + +Checking the current state +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The split is measurable rather than a matter of memory. Slice each ``_missing_`` body +and see which call it makes -- an :mod:`ast` walk is reliable where a text search is +not, because ``extend_enum`` also appears in imports and in prose: + +.. code-block:: python + + import ast, pathlib + + for path in sorted(pathlib.Path('pcapkit/const').rglob('*.py')): + if path.name == '__init__.py': + continue + tree = ast.parse(path.read_text()) + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef) and node.name == '_missing_': + calls = {n.func.id for n in ast.walk(node) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Name)} + if 'extend_enum' in calls: + print('MINT ', path) + elif '_unregistered_member' in calls: + print('UNMINT', path) + +.. warning:: + + Calling ``Cls(value)`` on a registry whose ``_missing_`` mints **mutates the + class**. A probe is not a read: it installs a member that every later lookup then + finds. Snapshot ``{member.value for member in Cls}`` before any lookup, and use a + throwaway process per registry when comparing behaviour across revisions. + diff --git a/docs/source/contributing/conventions.rst b/docs/source/contributing/conventions/registry-protocol.rst similarity index 51% rename from docs/source/contributing/conventions.rst rename to docs/source/contributing/conventions/registry-protocol.rst index d9f90306c..fad5742f6 100644 --- a/docs/source/contributing/conventions.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -1,312 +1,3 @@ -House Conventions -================= - -.. important:: - - 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 --------------------------------------------- - -Every registry under :mod:`pcapkit.const` defines ``_missing_``, which decides what -happens when a value has no member. There are two possible behaviours, and which one -a given range gets is a **design decision, not a style preference**: - -``extend_enum(cls, name, value)`` -- *mint* - Creates a real, permanent member on the class. It is installed in - ``_member_map_`` and ``_value2member_map_``, so it is visible to iteration, - lookup and ``__members__`` from then on, for the life of the process. - -:meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` -- *unmint* - Returns a member-like object for the value **without** installing it. The - registry does not grow, and a second lookup of the same value is indistinguishable - from the first. - -The criterion -~~~~~~~~~~~~~ - -The test, in the maintainer's words: - - Is this considered as the final concrete assigned name (**mint**), or just a - notation for the readers (**unmint**)? - -Settled on `#847 `__ and confirmed -as "a core concept of the ruling" on -`#775 `__. - -So the question to ask of a range is **what the upstream registry actually did**, not -what the generated code happens to look like: - -* The source assigns a **real, specific name** to those codes -- minting records - something the registry genuinely says. **Mint.** -* The source says only that the codes are spoken for, without naming them -- - ``Unassigned``, ``Reserved``, ``Reserved for Private Use``, - ``Reserved for Experimental Use``, ``Deprecated``, ``Dynamically Assigned`` - and ``Statically Assigned``. These are written for a human reading the - table. Minting them **manufactures a name nobody - assigned**, and the value will get its real name if and when something assigns - it. **Unmint.** - -Worked examples -~~~~~~~~~~~~~~~ - -*Unmint.* :mod:`pcapkit.const.ipx.socket`'s ``Dynamically Assigned``, -``Dynamically Assigned Socket Numbers``, ``Statically Assigned Socket Numbers`` and -``Experimental`` ranges. Each names **how the socket will be allocated**, not what -occupies it; the real name arrives with the allocation. - -*Mint.* :mod:`pcapkit.const.reg.ethertype`'s company names -- ``Xyplex``, -``Datability``, ``Qualcomm``, ``Motorola`` and forty-two others, 46 names across 50 -range blocks, since four of them hold two blocks each -- and, in the same file's -neighbour, :mod:`pcapkit.const.ipx.socket`'s ``Registered by Xerox``. A company name -is the assignment, for the reason in the next section. - -Two further ranges in that file mint without being company names at all: -``IEEE802.3 Length Field``, which names a field in a standard, and -``Berkeley Trailer encap/IP``, which names an encapsulation. Both are outside the 46, -and neither mints for the reason the next section gives. - -.. note:: - - Those two groups look alike and the line between them is **not** range-versus-single - code. ``Registered by Xerox`` covers a range and still mints, because it names *who - registered the socket*. ``Dynamically Assigned`` also covers a range and does not, - because it names only the *mechanism* by which some future party will take it. Ask - what the label tells you: a party, or a procedure. - -Why the company names mint -~~~~~~~~~~~~~~~~~~~~~~~~~~ - -The ethertype case looks like an exception to the rule and is not. The maintainer's -reasoning, settled on `#775 `__ after -being raised on `#847 `__: - - Proprietary protocols won't have public names so company names serve this purpose. - -So the company name is not a note *about* the code -- it is the best name that will ever -exist *for* it, which makes it the final concrete assigned name under the test above. -That is also why ``DEC Unassigned`` goes the other way despite carrying the same -attribution: there the company holds the block and assigned nothing, so the notation is -"Unassigned" and the attribution is incidental. - -Checking the current state -~~~~~~~~~~~~~~~~~~~~~~~~~~ - -The split is measurable rather than a matter of memory. Slice each ``_missing_`` body -and see which call it makes -- an :mod:`ast` walk is reliable where a text search is -not, because ``extend_enum`` also appears in imports and in prose: - -.. code-block:: python - - import ast, pathlib - - for path in sorted(pathlib.Path('pcapkit/const').rglob('*.py')): - if path.name == '__init__.py': - continue - tree = ast.parse(path.read_text()) - for node in ast.walk(tree): - if isinstance(node, ast.FunctionDef) and node.name == '_missing_': - calls = {n.func.id for n in ast.walk(node) - if isinstance(n, ast.Call) and isinstance(n.func, ast.Name)} - if 'extend_enum' in calls: - print('MINT ', path) - elif '_unregistered_member' in calls: - print('UNMINT', path) - -.. warning:: - - Calling ``Cls(value)`` on a registry whose ``_missing_`` mints **mutates the - class**. A probe is not a read: it installs a member that every later lookup then - finds. Snapshot ``{member.value for member in Cls}`` before any lookup, and use a - throwaway process per registry when comparing behaviour across revisions. - -.. _sentinel-convention: - -Naming a sentinel ------------------ - -A *sentinel* here is a module-level singleton whose only job is to be recognised by -identity -- ``value is SENTINEL`` -- so that it can never be confused with a value a -caller might legitimately pass. The house rule, from the maintainer, covers the type: - - Keep the sentinel object's type class naming as ``Type``. - -That is, the class takes the instance's name in CamelCase with ``Type`` appended. It -says nothing about the **object**'s own name, which is what let three casings diverge -with no rule naming any of them wrong. GitHub issue #937 closed that gap, verbatim: -*"take SCREAMING_SNAKE and accept the breaking change (no backport needed)."* So the -object is named in SCREAMING_SNAKE, and the type-naming rule above derives from it -mechanically -- title-case each underscore-separated word and append ``Type``, no -per-sentinel exception needed. The four in the tree follow it: - -.. list-table:: - :header-rows: 1 - :widths: 30 30 40 - - * - Instance - - Type - - Defined in - * - ``NULL`` - - ``NullType`` - - :mod:`pcapkit.corekit.sentinels` - * - ``NO_VALUE`` - - ``NoValueType`` - - :mod:`pcapkit.corekit.sentinels` - * - ``NO_DEFAULT`` - - ``NoDefaultType`` - - :mod:`pcapkit.corekit.sentinels` - * - ``ABSENT`` - - ``AbsentType`` - - :mod:`pcapkit.corekit.sentinels` - -All four used to live beside the one class that used them -- -:mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, -:mod:`pcapkit.corekit.enum` and :mod:`pcapkit.protocols.protocol` respectively. -GitHub issue #911's housing ruling, verbatim -- *"Okay one module for all four it -is."* -- moved the four definitions into the single shared module the table now -names; each original module keeps a re-export so every existing -``from import `` keeps working, including the -``if TYPE_CHECKING:``-only imports of the types. - -Before GitHub issue #937, the **instance** name's casing was deliberately free, which is -why ``NULL`` and ``NoValue`` disagreed and both were called correct -- three sentinels -had already picked three different casings (``NULL`` SCREAMING_SNAKE, ``NoValue`` -CamelCase, ``_Absent`` CamelCase with a leading underscore) before anyone ruled on it. -#937's ruling closes that: SCREAMING_SNAKE is now the one answer, and the two renames -it made -- ``NoValue`` to ``NO_VALUE``, ``_Absent`` to ``ABSENT`` -- are the breaking -change it accepted rather than deprecating. Where a sentinel name already exists and -already follows SCREAMING_SNAKE, keep it; renaming a published sentinel again costs -every caller for no further gain. - -The rename also dropped the **leading underscore** ``_Absent``/``_AbsentType`` used to -carry. ``ABSENT`` is private -- it is read in ``_declared_keywords`` and discarded -there, never leaving :mod:`pcapkit.protocols.protocol` -- and the underscore used to be -the mechanical signal of that. The maintainer's ruling on #937, verbatim: *"we can -change* ``_ABSENT`` *to* ``ABSENT`` *just document it as private type/class in the -documentation and not for public use is enough."* So privacy is documentation-only from -here on, carried by this paragraph and by -:class:`~pcapkit.corekit.sentinels.AbsentType`'s own docstring -(:file:`pcapkit/corekit/sentinels.py`, line 439), which still says so: - - A distinct class rather than a bare :obj:`object` so that the sentinel has a name - of its own in a traceback or a debugger, and so that a type checker has something - to name where ``object()`` would give it nothing. It follows - :class:`~pcapkit.corekit.sentinels.NoValueType`, which does the same job for an unset - field default; this is a sibling of it rather than a reuse [...] - -It remains a deliberate fourth rather than an accident: the leading underscore's -absence is also why this table once listed three for as long as it did -- a sweep -filtered on capitalised names did not see ``_Absent`` -- and that history does not -change now that nothing in the name itself marks it out. When adding a sentinel, add -it here whether or not it is public. - -What reaches users is the **object only**. The maintainer's ruling: *"we should ONLY -export the objects (like* ``NULL`` *) to users"* -- so a public sentinel names its -instance in its module's ``__all__`` and leaves the type out of it (GitHub issue #911). -The type stays importable by its dotted path, for an annotation or an ``is`` guard; it -is ``import *`` that no longer offers it. A private sentinel such as ``ABSENT`` is in -neither, which is what private means here -- dropping its leading underscore did not -add it to either list, and :class:`~pcapkit.corekit.sentinels.AbsentType` and -:data:`~pcapkit.corekit.sentinels.ABSENT` are documented on -:doc:`the sentinels API page ` as private and not for -public use rather than left off it, since the name alone no longer says so. - -.. note:: - - Of the four, only :class:`~pcapkit.corekit.sentinels.NullType` is a full worked - example. ``NoValueType`` follows the naming rule but is **not** a singleton - (``NoValueType() is NO_VALUE`` is :obj:`False`) and has no ``__repr__`` of its own, - so it demonstrates the name and nothing else; ``AbsentType`` has a ``__repr__`` - (````) but no singleton guard either. Copy ``NullType`` when you need a - pattern to follow. - -Why a class and not ``object()`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -A bare ``object()`` is just as safe under ``is``, so safety is not the reason. The -reason is legibility: a dedicated class can define ``__repr__``, and that repr is what -appears in a signature, in :func:`help` output and in a traceback. Compare what -:func:`inspect.signature` renders for a method whose default is the sentinel: - -.. code-block:: text - - # bare object(): an address, different every process - default: 'Any' = - - # dedicated type with __repr__ - default: 'Any' = - -.. warning:: - - Do **not** justify a dedicated class by claiming a subclass "could still compare - equal via a custom ``__eq__``". A class that defines only ``__repr__`` inherits - identity ``__eq__`` and is exactly as safe as ``object()``. That argument appeared - in an early draft of :mod:`pcapkit.corekit.enum` and was wrong. - -**Ported code is exempt.** ``_NOT_FOUND = object()`` at -:file:`pcapkit/utilities/compat.py`, line 73, sits inside the ``cached_property`` -backport taken for interpreters below 3.8, which tracks CPython's own -:mod:`functools` implementation down to that name. It is **not** to be converted: the -value of a vendored backport is that it can still be diffed against upstream, and a -house-style rewrite destroys that in exchange for a sentinel nobody outside those forty -lines ever sees. The rule above is for sentinels this package writes itself. - -What to implement, and what not to -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -The four sentinels deliberately differ, and the differences are **needs, not -inconsistencies**: - -``__new__`` returning a cached instance - Guards against a caller constructing a second, non-identical sentinel that then - fails every ``is`` check. Worth having wherever the type is reachable by a caller at - all -- which, since the type is kept out of ``__all__``, means wherever it is - importable by its dotted path rather than wherever it is star-exported. - :class:`~pcapkit.corekit.sentinels.NullType` documents the limit honestly: a module - **reload** re-executes the class statement, so the guard does not survive one, and - code holding the pre-reload instance will fail ``is``. Since GitHub issue #911, - that means reloading :mod:`pcapkit.corekit.sentinels` itself -- reloading - :mod:`pcapkit.corekit.module`, which now only re-exports the sentinel, no longer - has any effect on it. - -``__bool__`` returning :obj:`False` - ``NULL``, ``NO_VALUE`` and ``ABSENT`` have it, because each stands for an *absent - value* and reads naturally in a boolean test. ``NO_DEFAULT`` deliberately does - **not**: it is a marker meaning *no default was supplied*, it is only ever tested - with ``is``, and making it falsy would invite ``if not default:`` -- which would - then treat a caller's genuine falsy default (``0``, ``''``, :obj:`None`, - :obj:`False`) the same as the sentinel, the very confusion the sentinel exists to - prevent. - -``__copy__`` / ``__deepcopy__`` / ``__reduce__`` - :class:`~pcapkit.corekit.sentinels.NullType` has them because ``NULL`` is stored in a - :class:`~pcapkit.corekit.module.ModuleDescriptor` field, so a caller's - :func:`copy.deepcopy` or :mod:`pickle` can walk into it and would otherwise - reconstruct a second instance. ``NO_DEFAULT`` and ``ABSENT`` have none, because - neither is ever stored in any structure a caller copies -- one only ever appears as - a default argument, and the other never leaves the module that reads it. - Add them when, and only when, the sentinel becomes reachable from something - copyable. - .. _registry-protocol: Where the registry protocol lives @@ -775,192 +466,3 @@ reasoning. :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/contributing/conventions/sentinel-convention.rst b/docs/source/contributing/conventions/sentinel-convention.rst new file mode 100644 index 000000000..84f822999 --- /dev/null +++ b/docs/source/contributing/conventions/sentinel-convention.rst @@ -0,0 +1,168 @@ +.. _sentinel-convention: + +Naming a sentinel +----------------- + +A *sentinel* here is a module-level singleton whose only job is to be recognised by +identity -- ``value is SENTINEL`` -- so that it can never be confused with a value a +caller might legitimately pass. The house rule, from the maintainer, covers the type: + + Keep the sentinel object's type class naming as ``Type``. + +That is, the class takes the instance's name in CamelCase with ``Type`` appended. It +says nothing about the **object**'s own name, which is what let three casings diverge +with no rule naming any of them wrong. GitHub issue #937 closed that gap, verbatim: +*"take SCREAMING_SNAKE and accept the breaking change (no backport needed)."* So the +object is named in SCREAMING_SNAKE, and the type-naming rule above derives from it +mechanically -- title-case each underscore-separated word and append ``Type``, no +per-sentinel exception needed. The four in the tree follow it: + +.. list-table:: + :header-rows: 1 + :widths: 30 30 40 + + * - Instance + - Type + - Defined in + * - ``NULL`` + - ``NullType`` + - :mod:`pcapkit.corekit.sentinels` + * - ``NO_VALUE`` + - ``NoValueType`` + - :mod:`pcapkit.corekit.sentinels` + * - ``NO_DEFAULT`` + - ``NoDefaultType`` + - :mod:`pcapkit.corekit.sentinels` + * - ``ABSENT`` + - ``AbsentType`` + - :mod:`pcapkit.corekit.sentinels` + +All four used to live beside the one class that used them -- +:mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, +:mod:`pcapkit.corekit.enum` and :mod:`pcapkit.protocols.protocol` respectively. +GitHub issue #911's housing ruling, verbatim -- *"Okay one module for all four it +is."* -- moved the four definitions into the single shared module the table now +names; each original module keeps a re-export so every existing +``from import `` keeps working, including the +``if TYPE_CHECKING:``-only imports of the types. + +Before GitHub issue #937, the **instance** name's casing was deliberately free, which is +why ``NULL`` and ``NoValue`` disagreed and both were called correct -- three sentinels +had already picked three different casings (``NULL`` SCREAMING_SNAKE, ``NoValue`` +CamelCase, ``_Absent`` CamelCase with a leading underscore) before anyone ruled on it. +#937's ruling closes that: SCREAMING_SNAKE is now the one answer, and the two renames +it made -- ``NoValue`` to ``NO_VALUE``, ``_Absent`` to ``ABSENT`` -- are the breaking +change it accepted rather than deprecating. Where a sentinel name already exists and +already follows SCREAMING_SNAKE, keep it; renaming a published sentinel again costs +every caller for no further gain. + +The rename also dropped the **leading underscore** ``_Absent``/``_AbsentType`` used to +carry. ``ABSENT`` is private -- it is read in ``_declared_keywords`` and discarded +there, never leaving :mod:`pcapkit.protocols.protocol` -- and the underscore used to be +the mechanical signal of that. The maintainer's ruling on #937, verbatim: *"we can +change* ``_ABSENT`` *to* ``ABSENT`` *just document it as private type/class in the +documentation and not for public use is enough."* So privacy is documentation-only from +here on, carried by this paragraph and by +:class:`~pcapkit.corekit.sentinels.AbsentType`'s own docstring +(:file:`pcapkit/corekit/sentinels.py`, line 439), which still says so: + + A distinct class rather than a bare :obj:`object` so that the sentinel has a name + of its own in a traceback or a debugger, and so that a type checker has something + to name where ``object()`` would give it nothing. It follows + :class:`~pcapkit.corekit.sentinels.NoValueType`, which does the same job for an unset + field default; this is a sibling of it rather than a reuse [...] + +It remains a deliberate fourth rather than an accident: the leading underscore's +absence is also why this table once listed three for as long as it did -- a sweep +filtered on capitalised names did not see ``_Absent`` -- and that history does not +change now that nothing in the name itself marks it out. When adding a sentinel, add +it here whether or not it is public. + +What reaches users is the **object only**. The maintainer's ruling: *"we should ONLY +export the objects (like* ``NULL`` *) to users"* -- so a public sentinel names its +instance in its module's ``__all__`` and leaves the type out of it (GitHub issue #911). +The type stays importable by its dotted path, for an annotation or an ``is`` guard; it +is ``import *`` that no longer offers it. A private sentinel such as ``ABSENT`` is in +neither, which is what private means here -- dropping its leading underscore did not +add it to either list, and :class:`~pcapkit.corekit.sentinels.AbsentType` and +:data:`~pcapkit.corekit.sentinels.ABSENT` are documented on +:doc:`the sentinels API page ` as private and not for +public use rather than left off it, since the name alone no longer says so. + +.. note:: + + Of the four, only :class:`~pcapkit.corekit.sentinels.NullType` is a full worked + example. ``NoValueType`` follows the naming rule but is **not** a singleton + (``NoValueType() is NO_VALUE`` is :obj:`False`) and has no ``__repr__`` of its own, + so it demonstrates the name and nothing else; ``AbsentType`` has a ``__repr__`` + (````) but no singleton guard either. Copy ``NullType`` when you need a + pattern to follow. + +Why a class and not ``object()`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +A bare ``object()`` is just as safe under ``is``, so safety is not the reason. The +reason is legibility: a dedicated class can define ``__repr__``, and that repr is what +appears in a signature, in :func:`help` output and in a traceback. Compare what +:func:`inspect.signature` renders for a method whose default is the sentinel: + +.. code-block:: text + + # bare object(): an address, different every process + default: 'Any' = + + # dedicated type with __repr__ + default: 'Any' = + +.. warning:: + + Do **not** justify a dedicated class by claiming a subclass "could still compare + equal via a custom ``__eq__``". A class that defines only ``__repr__`` inherits + identity ``__eq__`` and is exactly as safe as ``object()``. That argument appeared + in an early draft of :mod:`pcapkit.corekit.enum` and was wrong. + +**Ported code is exempt.** ``_NOT_FOUND = object()`` at +:file:`pcapkit/utilities/compat.py`, line 73, sits inside the ``cached_property`` +backport taken for interpreters below 3.8, which tracks CPython's own +:mod:`functools` implementation down to that name. It is **not** to be converted: the +value of a vendored backport is that it can still be diffed against upstream, and a +house-style rewrite destroys that in exchange for a sentinel nobody outside those forty +lines ever sees. The rule above is for sentinels this package writes itself. + +What to implement, and what not to +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The four sentinels deliberately differ, and the differences are **needs, not +inconsistencies**: + +``__new__`` returning a cached instance + Guards against a caller constructing a second, non-identical sentinel that then + fails every ``is`` check. Worth having wherever the type is reachable by a caller at + all -- which, since the type is kept out of ``__all__``, means wherever it is + importable by its dotted path rather than wherever it is star-exported. + :class:`~pcapkit.corekit.sentinels.NullType` documents the limit honestly: a module + **reload** re-executes the class statement, so the guard does not survive one, and + code holding the pre-reload instance will fail ``is``. Since GitHub issue #911, + that means reloading :mod:`pcapkit.corekit.sentinels` itself -- reloading + :mod:`pcapkit.corekit.module`, which now only re-exports the sentinel, no longer + has any effect on it. + +``__bool__`` returning :obj:`False` + ``NULL``, ``NO_VALUE`` and ``ABSENT`` have it, because each stands for an *absent + value* and reads naturally in a boolean test. ``NO_DEFAULT`` deliberately does + **not**: it is a marker meaning *no default was supplied*, it is only ever tested + with ``is``, and making it falsy would invite ``if not default:`` -- which would + then treat a caller's genuine falsy default (``0``, ``''``, :obj:`None`, + :obj:`False`) the same as the sentinel, the very confusion the sentinel exists to + prevent. + +``__copy__`` / ``__deepcopy__`` / ``__reduce__`` + :class:`~pcapkit.corekit.sentinels.NullType` has them because ``NULL`` is stored in a + :class:`~pcapkit.corekit.module.ModuleDescriptor` field, so a caller's + :func:`copy.deepcopy` or :mod:`pickle` can walk into it and would otherwise + reconstruct a second instance. ``NO_DEFAULT`` and ``ABSENT`` have none, because + neither is ever stored in any structure a caller copies -- one only ever appears as + a default argument, and the other never leaves the module that reads it. + Add them when, and only when, the sentinel becomes reachable from something + copyable. + diff --git a/docs/source/index.rst b/docs/source/index.rst index 74d5fd654..91b1fb0a9 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -35,7 +35,7 @@ construction and analysis library. :maxdepth: 1 contributing/testing - contributing/conventions + contributing/conventions/index contributing/releasing contributing/workflows contributing/pep diff --git a/pcapkit/corekit/sentinels.py b/pcapkit/corekit/sentinels.py index 03c1aef67..59ff330b7 100644 --- a/pcapkit/corekit/sentinels.py +++ b/pcapkit/corekit/sentinels.py @@ -9,8 +9,8 @@ whose only job is to be recognised by identity (``value is SENTINEL``), so that it can never be confused with a value a caller might legitimately pass. See the "Naming a sentinel" section of -:file:`docs/source/contributing/conventions.rst` for the house rule the four -below follow. +:file:`docs/source/contributing/conventions/sentinel-convention.rst` for the +house rule the four below follow. Before this module existed, each of the four lived beside the one class that used it: :class:`NullType` in :mod:`pcapkit.corekit.module`, @@ -461,8 +461,8 @@ class AbsentType: :data:`ABSENT`, from here or from there, and neither this module's nor that module's :attr:`__all__` names either one. This docstring, and the "Naming a sentinel" section of - :file:`docs/source/contributing/conventions.rst`, are what now records - that fact in place of the leading underscore. + :file:`docs/source/contributing/conventions/sentinel-convention.rst`, are + what now records that fact in place of the leading underscore. """ diff --git a/tests/corekit/test_enum_lookup_base_unit.py b/tests/corekit/test_enum_lookup_base_unit.py index 2c06f0b74..6b778d90b 100644 --- a/tests/corekit/test_enum_lookup_base_unit.py +++ b/tests/corekit/test_enum_lookup_base_unit.py @@ -213,9 +213,9 @@ def test_str_lookup_by_name_and_by_value(self) -> 'None': """Including a value that is not also a member name. ``_Str.get('')`` is the measurement - :file:`docs/source/contributing/conventions.rst` records on - :class:`~pcapkit.const.ftp.command.FEATCode`, made here on a class that - cannot be overriding ``get``, since it defines none. + :file:`docs/source/contributing/conventions/registry-protocol.rst` + records on :class:`~pcapkit.const.ftp.command.FEATCode`, made here on a + class that cannot be overriding ``get``, since it defines none. """ self.assertIs(_Str.get('plain'), _Str.plain) diff --git a/tests/corekit/test_fields_numbers_unassigned_enum.py b/tests/corekit/test_fields_numbers_unassigned_enum.py index 1c2929114..c1ae17d8e 100644 --- a/tests/corekit/test_fields_numbers_unassigned_enum.py +++ b/tests/corekit/test_fields_numbers_unassigned_enum.py @@ -184,7 +184,8 @@ def test_a_missing_rule_still_takes_precedence_over_the_fallback(self) -> None: ``0x0bad0bad`` is in one of the ``Reserved_*`` ranges :meth:`BlockType._missing_ ` covers. Per the - mint/unmint ruling recorded for #775 (``docs/source/contributing/conventions.rst``), + mint/unmint ruling recorded for #775 + (``docs/source/contributing/conventions/mint-criterion.rst``), ``Reserved`` names a procedure rather than a party, so this range no longer *mints* a registered ``Reserved_0bad0bad`` member -- it now returns an unregistered member via ``_unregistered_member``, bearing the diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py index 58a62649a..db38db10f 100644 --- a/tests/corekit/test_sentinel_exports_unit.py +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -24,7 +24,8 @@ pins it so a later reading of the ruling cannot escalate into deleting the types. The population is **four**, not the three -:file:`docs/source/contributing/conventions.rst` documented -- ``ABSENT`` / +:file:`docs/source/contributing/conventions/sentinel-convention.rst` documented -- +``ABSENT`` / ``AbsentType`` in :mod:`pcapkit.protocols.protocol` is the fourth, missed because a sweep filtered on capitalised names did not see it when it was still spelled ``_Absent``/``_AbsentType``, with a leading underscore. :class:`SentinelPopulationTests` @@ -127,7 +128,8 @@ #: Every sentinel in the tree that follows the house ``Type`` convention, #: as ``(instance name, instance, type)``. Four, not the three -#: :file:`docs/source/contributing/conventions.rst` used to document -- see the module docstring. +#: :file:`docs/source/contributing/conventions/sentinel-convention.rst` used to +#: document -- see the module docstring. #: All four now share :data:`CANONICAL_MODULE` as their defining module, which is #: why a per-entry module column is no longer part of this tuple -- see #: :data:`PUBLIC_SENTINELS` below for the (still distinct) *shim* locations. @@ -179,25 +181,25 @@ def _star_import(module: 'str') -> 'dict[str, object]': def _sentinel_section() -> 'str': - """The "Naming a sentinel" section of :file:`docs/source/contributing/conventions.rst`. - - Sliced by its own section markers rather than by line number, so an edit - elsewhere in the file does not silently make this read the wrong text. That - is what survived GitHub pull request #912 moving the file out of - :file:`docs/source/` into :file:`docs/source/contributing/`, which landed as - ``9806f16aa``; the two-element candidate tuple that straddled the move is - gone with it (GitHub issue #920), since its first entry could never match - again and read as though both locations were still live. + """The "Naming a sentinel" section, now its own page. + + Read whole rather than sliced between two anchors: GitHub issue #918 split + the single-page :file:`docs/source/contributing/conventions.rst` into one + file per ``.. _label:`` anchor, and the "Naming a sentinel" section *is* + :file:`docs/source/contributing/conventions/sentinel-convention.rst` now, so + there is no following anchor left in the same file to slice against. That + retires the ``text.index('.. _sentinel-convention:')`` / + ``text.index('.. _registry-protocol:', start)`` pairing this used before the + split -- the two anchors moved into separate files, which is exactly what + GitHub issue #930 flagged as the split's concrete blocker before #918 + resolved it. """ - path = ROOT / 'docs/source/contributing/conventions.rst' + path = ROOT / 'docs/source/contributing/conventions/sentinel-convention.rst' if not path.is_file(): # pragma: no cover - raise AssertionError(f'conventions.rst not found at {path}') + raise AssertionError(f'sentinel-convention.rst not found at {path}') - text = path.read_text(encoding='utf-8') - start = text.index('.. _sentinel-convention:') - end = text.index('.. _registry-protocol:', start) - return text[start:end] + return path.read_text(encoding='utf-8') class SentinelExportTests(unittest.TestCase): @@ -221,8 +223,10 @@ def test_enum_exports_the_object_and_not_the_type(self) -> 'None': """``EnumLookup`` and ``EnumRegistry`` are not sentinels and stay. ``EnumLookup`` in particular: GitHub issue #906 split it out as a public - base and :file:`docs/source/contributing/conventions.rst` cites its ``get``, so dropping - it while removing the sentinel type next to it would break that reference. + base and + :file:`docs/source/contributing/conventions/registry-protocol.rst` cites + its ``get``, so dropping it while removing the sentinel type next to it + would break that reference. """ import pcapkit.corekit.enum as enum @@ -407,7 +411,9 @@ def test_the_sentinel_table_has_a_row_per_sentinel_and_no_more(self) -> 'None': class SentinelBehaviourTests(unittest.TestCase): - """The per-sentinel differences :file:`docs/source/contributing/conventions.rst` documents. + """The per-sentinel differences + :file:`docs/source/contributing/conventions/sentinel-convention.rst` + documents. Not part of the export change, and asserted here because the doc edit that goes with it makes claims about all four -- an undocumented ``__bool__`` or a missing diff --git a/tests/project/test_conventions_doc_claims.py b/tests/project/test_conventions_doc_claims.py index 50858f906..516bad25c 100644 --- a/tests/project/test_conventions_doc_claims.py +++ b/tests/project/test_conventions_doc_claims.py @@ -1,20 +1,28 @@ # -*- 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. +"""Claims the split *House Conventions* pages make that the tree can be asked about. + +:file:`docs/source/contributing/conventions.rst` used to record every design ruling +on one 966-line page, 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 part 1 harvested the settled rulings onto that page; part 2 then +split it into :file:`docs/source/contributing/conventions/`, one file per +``.. _label:`` anchor plus the :file:`index.rst` that carries the ``.. important::`` +preamble and the toctree. This file pins the checkable claims, plus the split's own +structure: + +* :class:`ConventionAnchorTests` -- the four ``.. _label:`` anchors, one now per + file. Before the split all four lived on one page, and + ``tests/corekit/test_sentinel_exports_unit.py`` sliced the file *between* two of + them -- which is exactly what GitHub issue #930 named as the split's concrete + blocker, since separating those two anchors into different files made that slice + raise :exc:`ValueError`. This class pins the post-split shape: every anchor still + exists, lives in exactly its own file, is listed in the index's toctree, and the + top-level :file:`docs/source/index.rst` points at the new index page rather than + the retired bare document path. * :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 @@ -36,7 +44,8 @@ fix alongside it: the four sentinel references #934 part B qualified stay qualified, since none of ``AbsentType``, ``NoValueType`` or ``ABSENT`` resolves unqualified outside :file:`docs/source/pcapkit/corekit/sentinels.rst`'s own module - context. + context. Both checks scan every split page rather than one file, since either + could in principle land on any of them. 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 @@ -58,14 +67,21 @@ 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. +#: The directory GitHub issue #918 part 2 split the single-page +#: :file:`conventions.rst` into. Hard-coded rather than discovered, because the path +#: is *also* what ``pcapkit/corekit/sentinels.py`` and +#: ``tests/corekit/test_sentinel_exports_unit.py`` hard-code -- so if the layout +#: moves again, every one of them has to be updated together, and a test that found +#: it either way would hide that. +CONVENTIONS_DIR = ROOT / 'docs' / 'source' / 'contributing' / 'conventions' + +#: The index page that carries the toctree and the ``.. important::`` preamble the +#: single page used to open with. +INDEX = CONVENTIONS_DIR / 'index.rst' + +#: Every ``.. _label:`` anchor, in the narrative order the pre-split page carried +#: them in -- which is also the order the index's toctree lists the files in. The +#: first three predate #918; ``extension-header-subclassing`` arrived with it. ANCHORS = ( 'mint-criterion', 'sentinel-convention', @@ -73,6 +89,10 @@ 'extension-header-subclassing', ) +#: Each anchor's own file, one-to-one since the split -- there is no longer a single +#: shared page to slice between two of them. +PAGES = {anchor: CONVENTIONS_DIR / f'{anchor}.rst' for anchor in ANCHORS} + #: 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. #: ``'zero'`` joined the set with GitHub issue #930, once phase 2 finished and the @@ -83,51 +103,78 @@ } -def _page() -> 'str': - """The page's text. +def _page(anchor: 'str') -> 'str': + """``anchor``'s own page, whole. + + The split retired the anchor-to-anchor slice this used to need: each anchor is + now the whole of its own file, rather than a range between two markers in one + shared page. ``tests/corekit/test_sentinel_exports_unit.py``'s + ``_sentinel_section`` made the same change, for the same reason. + + Args: + anchor: One of :data:`ANCHORS`. + + Returns: + The page's text. Raises: - AssertionError: If the page is not where every reference to it says it is. + AssertionError: If the page is not where the split put it. """ - if not CONVENTIONS.is_file(): # pragma: no cover + path = PAGES[anchor] + if not path.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' + f'{path.name} not found at {path}; GitHub issue #918 part 2 split it ' + 'out of the single-page conventions.rst -- pcapkit/corekit/sentinels.py ' + 'and tests/corekit/test_sentinel_exports_unit.py both name paths under ' + 'this directory too' ) - return CONVENTIONS.read_text(encoding='utf-8') + return path.read_text(encoding='utf-8') -def _section(anchor: 'str') -> 'str': - """The page text from ``anchor`` up to the next anchor, or to the end. +def _every_page() -> 'str': + """Every split page's text, concatenated, index included. - 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. + For the checks that used to scan the single-page file end to end -- a forbidden + role, a qualified reference -- and still need to scan across all four sections + plus the preamble, since either could in principle land on any of them. - 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. + """ + return '\n'.join([INDEX.read_text(encoding='utf-8')] + + [_page(anchor) for anchor in ANCHORS]) - Args: - anchor: The label to slice from, without the ``.. _`` and ``:``. - Returns: - The section's text. +def _toctree_entries(text: 'str') -> 'list[str]': + """The entries of the first ``.. toctree::`` directive in ``text``. - Raises: - AssertionError: If ``anchor`` is not on the page at all. + Parsed structurally -- skip the directive's own options (``:maxdepth:`` and the + like), then collect non-blank lines until the entry block ends -- rather than + searched for a literal substring, following + ``tests/project/test_sentinels_doc_page_934_unit.py``'s own copy of this helper, + so a reordering or an added option does not misreport what the toctree actually + names. """ - 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:] + lines = text.splitlines() + for index, line in enumerate(lines): + if line.strip() == '.. toctree::': + break + else: + raise AssertionError('no ".. toctree::" directive found') + + entries = [] # type: list[str] + started = False + for line in lines[index + 1:]: + stripped = line.strip() + if not stripped: + if started: + break + continue + if stripped.startswith(':'): + continue + started = True + entries.append(stripped) + return entries def _every_enumeration() -> 'dict[type, str]': @@ -199,38 +246,126 @@ def walk(container: 'type') -> 'None': class ConventionAnchorTests(unittest.TestCase): - """The labels other files cross-reference, all in the one page.""" + """The labels other files cross-reference, one now per file.""" + + def test_every_page_exists(self) -> 'None': + """The split's own four files, plus the index, are all on disk.""" + for path in [INDEX] + [PAGES[anchor] for anchor in ANCHORS]: + with self.subTest(page=path.name): + self.assertTrue(path.is_file(), f'{path} does not exist') 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. + reference is not a build failure -- it is a word that used to be a link. + Checked against each anchor's own file, since the split gave each anchor + exactly one home rather than one shared page. """ - 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 + text = _page(anchor) + # ``assertTrue`` rather than ``assertIn``: the latter dumps the + # whole 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}') + f'.. _{anchor}: is no longer on {PAGES[anchor].name}') + + def test_no_page_carries_a_different_page_s_anchor(self) -> 'None': + """Each anchor lives in exactly one file -- the split's whole point. + + Before the split, ``tests/corekit/test_sentinel_exports_unit.py`` sliced one + shared page between ``.. _sentinel-convention:`` and + ``.. _registry-protocol:`` -- reading ``text.index('.. _sentinel-convention:')`` + through ``text.index('.. _registry-protocol:', start)``. GitHub issue #930 + named separating those two anchors into different files as the split's + concrete blocker, because that slice would then raise :exc:`ValueError` from + :meth:`str.index` rather than a wrong answer. #918 part 2 retired the slice + instead of working around it -- ``_sentinel_section`` now reads + :file:`sentinel-convention.rst` whole. What used to be a hard constraint on + the single page is now a fact about the four files, pinned here so a later + merge back into one page, or a bad copy-paste across two of them, does not + silently resurrect it. + + """ + for anchor in ANCHORS: + text = _page(anchor) + for other in ANCHORS: + if other == anchor: + continue + with self.subTest(page=anchor, other_anchor=other): + self.assertNotIn(f'.. _{other}:', text, + f'{PAGES[anchor].name} carries .. _{other}:, ' + f'which belongs in {PAGES[other].name}') + + def test_the_index_toctree_lists_every_page_in_order(self) -> 'None': + """The index's toctree is what keeps every page from being an orphan. + + An orphan page still resolves cross-references -- Sphinx's reference + inventory does not care whether a page is reachable from a toctree -- but it + produces a distinct "document isn't included in any toctree" warning. Order + matches :data:`ANCHORS`, the narrative order the single page used to carry + the four sections in. + + """ + entries = _toctree_entries(INDEX.read_text(encoding='utf-8')) + self.assertEqual(entries, list(ANCHORS), + f'{INDEX} toctree lists {entries!r}, expected ' + f'{list(ANCHORS)!r}') - 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. + def test_the_top_level_index_points_at_the_new_index_page(self) -> 'None': + """:file:`docs/source/index.rst` has to name a document, not a directory. - 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. + ``.. toctree::`` entries are docnames, and the bare ``contributing/conventions`` + entry it used to carry stopped being one the moment the split turned that + path into a directory -- Sphinx would report "unknown document" for it, + silently, since the build here runs without ``-W``/``-n``. Checked + structurally rather than trusted to a nitpicky build alone. + + """ + text = (ROOT / 'docs' / 'source' / 'index.rst').read_text(encoding='utf-8') + self.assertIn('contributing/conventions/index', text, + 'docs/source/index.rst no longer points at the split index ' + 'page') + self.assertNotRegex(text, r'(?m)^\s+contributing/conventions\s*$', + 'docs/source/index.rst still names the retired bare ' + '"contributing/conventions" path, which is now a ' + 'directory rather than a document') + + def test_the_index_preamble_does_not_claim_to_be_a_ruling_page_itself(self) -> 'None': + """The index's own prose has to read true of a hub, not of a page. + + Round 1 of #918 part 2's split moved the ``.. important::`` preamble onto + :data:`INDEX` unedited, and its prose was written when the whole thing was + one page: *"This page records design rulings"*, and a future one *"is + written onto this page"*. Both went false the moment the index stopped + carrying any ruling of its own -- the four children do -- and the second + is worse than stale, because it is the standing instruction #918 part 3 + exists to keep alive, now telling a contributor to write onto the wrong + file. A cross-review caught this on the first PR revision. + + Checked as a ban on the self-referential singular ``"this page"`` rather + than against the exact old sentences, so a future rewrite that + reintroduces the same mistake in different words still trips this -- the + index legitimately never needs that phrase, since every true statement + about ruling content here names a child page, or says "the pages below" + / "here" for the set of them. Paired with a positive check that the + corrected standing-instruction phrase actually landed, rather than merely + that the old one is gone, following :class:`RetiredNameTests`'s two-sided + pattern for a retired name elsewhere in this module. """ - text = _page() - self.assertLess(text.index('.. _sentinel-convention:'), - text.index('.. _registry-protocol:')) + text = INDEX.read_text(encoding='utf-8') + self.assertNotIn('this page', text.lower(), + f'{INDEX} claims something about "this page" -- the ' + 'index carries no ruling of its own, so nothing on it ' + 'should read as self-referential') + self.assertIn('the page that covers it', text, + f'{INDEX} no longer points a future ruling at "the page ' + 'that covers it"; the standing #918 instruction to ' + 'document a ruling in the same change that implements it ' + 'is pointing at the wrong target again') class PhaseTwoRemainderTests(unittest.TestCase): @@ -246,7 +381,7 @@ def setUp(self) -> 'None': 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()) + self.note = ' '.join(_page('registry-protocol').split()) def test_the_walk_found_something_to_count(self) -> 'None': """Guards every count below from passing on an empty discovery.""" @@ -342,7 +477,7 @@ def _table_rows() -> 'list[list[str]]': One list of cell strings per row. """ - section = _section('extension-header-subclassing') + section = _page('extension-header-subclassing') lines = section[section.index('.. list-table::'):].splitlines() rows = [] # type: list[list[str]] @@ -466,7 +601,7 @@ def test_the_page_names_the_exception_the_code_actually_raises(self) -> 'None': FEATCode.get('ZZ-NOT-REAL') self.assertIsInstance(caught.exception, KeyError) - section = ' '.join(_section('registry-protocol').split()) + section = ' '.join(_page('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 ' @@ -522,35 +657,40 @@ def test_no_aenum_role_appears(self) -> 'None': Checked as a role, not as the bare word: ``aenum`` still appears as a plain double-backtick literal (``` ``aenum.Enum`` ```) and in prose - (*"the aenum flavours"*) throughout this page, which is exactly the point - of the fix -- only the unresolvable *role* form is banned. + (*"the aenum flavours"*) throughout these pages, which is exactly the + point of the fix -- only the unresolvable *role* form is banned. Scanned + across every split page plus the index, since the six roles #934 part C + found could each have landed in any of them. """ - text = _page() + text = _every_page() offenders = self.FORBIDDEN_AENUM_ROLE.findall(text) self.assertEqual(offenders, [], - f'{CONVENTIONS.name} names aenum in a role again: ' - f'{offenders!r}; GitHub issue #934 part C ruled this ' - 'unresolvable (conf.py excludes aenum -- zero py: objects ' - 'in its objects.inv) and converted every such role to a ' - 'plain literal') + f'the split conventions pages name aenum in a role ' + f'again: {offenders!r}; GitHub issue #934 part C ruled ' + 'this unresolvable (conf.py excludes aenum -- zero py: ' + 'objects in its objects.inv) and converted every such ' + 'role to a plain literal') def test_the_qualified_sentinel_targets_stay_qualified(self) -> 'None': """A later edit unqualifying one of these reintroduces #934 part B's miss. ``AbsentType``, ``NoValueType`` and ``ABSENT`` each resolve only against the sentinels page's own module context (GitHub issue #936); written bare - anywhere on this page, none of the three resolves at all. + anywhere on these pages, none of the three resolves at all. All four + qualified references happen to live in + :file:`sentinel-convention.rst`, but this scans every split page plus the + index so a later move of one does not go unnoticed. """ - text = _page() + text = _every_page() for role, target, count in self.QUALIFIED_SENTINEL_REFS: with self.subTest(target=target): needle = f'{role}`{target}`' actual = text.count(needle) self.assertEqual(actual, count, - f'{needle!r} appears {actual} time(s) in ' - f'{CONVENTIONS.name}, expected {count}') + f'{needle!r} appears {actual} time(s) across ' + f'the split conventions pages, expected {count}') if __name__ == '__main__': diff --git a/tests/project/test_ftp_featcode_doc_page_934_unit.py b/tests/project/test_ftp_featcode_doc_page_934_unit.py index b1d20ec1f..531d115fc 100644 --- a/tests/project/test_ftp_featcode_doc_page_934_unit.py +++ b/tests/project/test_ftp_featcode_doc_page_934_unit.py @@ -1,9 +1,12 @@ # -*- coding: utf-8 -*- """Pins :class:`~pcapkit.const.ftp.command.FEATCode` as a documented API target. -GitHub issue #934, part B: :file:`docs/source/contributing/conventions.rst` -cross-references :class:`~pcapkit.const.ftp.command.FEATCode` three times (the -``:class:`` role, at what were lines 592, 733 and 740 on ``83c7552b8``), and none of +GitHub issue #934, part B: what is now +:file:`docs/source/contributing/conventions/registry-protocol.rst` (GitHub issue +#918 split it out of the single-page :file:`conventions.rst` that carried it at +the time) cross-references :class:`~pcapkit.const.ftp.command.FEATCode` three +times (the ``:class:`` role, at what were lines 592, 733 and 740 on +``83c7552b8``), and none of them used to resolve, because no page under :file:`docs/source/` documented that class -- confirmed by an explicit nitpicky ``sphinx-build`` before :file:`docs/source/pcapkit/const/ftp.rst` gained an ``autoclass`` directive for it, @@ -46,10 +49,10 @@ def test_page_documents_the_featcode_class(self) -> 'None': """The page declares an ``autoclass`` for the class conventions.rst names. Without this directive, ``:class:`~pcapkit.const.ftp.command.FEATCode``` - in :file:`docs/source/contributing/conventions.rst` has no target to - resolve against -- the exact defect GitHub issue #934 reports for this - class, distinct from the sibling ``sentinels`` module-level miss part A - of the same issue fixed. + in :file:`docs/source/contributing/conventions/registry-protocol.rst` + has no target to resolve against -- the exact defect GitHub issue #934 + reports for this class, distinct from the sibling ``sentinels`` + module-level miss part A of the same issue fixed. """ text = PAGE.read_text(encoding='utf-8') diff --git a/tests/project/test_sentinels_doc_page_934_unit.py b/tests/project/test_sentinels_doc_page_934_unit.py index 1c76aa4f6..013597bd8 100644 --- a/tests/project/test_sentinels_doc_page_934_unit.py +++ b/tests/project/test_sentinels_doc_page_934_unit.py @@ -1,11 +1,14 @@ # -*- coding: utf-8 -*- """Pins the API page for :mod:`pcapkit.corekit.sentinels` existing and reachable. -GitHub issue #934: :file:`docs/source/contributing/conventions.rst` cross-references -:mod:`pcapkit.corekit.sentinels` five times (the ``:mod:`` role, at lines 165, 168, 171, -174 and 261), and none of them used to resolve, because no page under -:file:`docs/source/` documented that module -- confirmed by an explicit nitpicky -``sphinx-build`` before this page existed, and again after, to confirm the fix. +GitHub issue #934: what is now +:file:`docs/source/contributing/conventions/sentinel-convention.rst` (GitHub +issue #918 split it out of the single-page :file:`conventions.rst` that carried +it at the time) cross-references :mod:`pcapkit.corekit.sentinels` five times +(the ``:mod:`` role, at lines 165, 168, 171, 174 and 261 of the pre-split page), +and none of them used to resolve, because no page under :file:`docs/source/` +documented that module -- confirmed by an explicit nitpicky ``sphinx-build`` +before this page existed, and again after, to confirm the fix. This does **not** re-test Sphinx's cross-reference resolution itself -- as :file:`tests/project/test_documentation_claims.py` explains at length, whether a diff --git a/tests/protocols/internet/test_ah_unit.py b/tests/protocols/internet/test_ah_unit.py index 619690b18..42d8f3446 100644 --- a/tests/protocols/internet/test_ah_unit.py +++ b/tests/protocols/internet/test_ah_unit.py @@ -131,8 +131,9 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: appear after hop-by-hop, routing, and fragmentation extension headers"), not a membership assertion -- membership is the registry's claim, per - ``docs/source/contributing/conventions.rst``'s "being *in* that - registry is what makes something an extension header" ruling. + ``docs/source/contributing/conventions/extension-header-subclassing.rst``'s + "being *in* that registry is what makes something an extension + header" ruling. Rephrasing the placement sentence as "places it among the IPv6 extension headers" overstated the RFC; that was caught in review on the second version of this fix. @@ -143,8 +144,9 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: also travels directly as an IPv4 payload, exactly as :mod:`~pcapkit.protocols.internet.hip` cites its own IPv4 appendix for the same reason. Per - ``docs/source/contributing/conventions.rst``'s "own-protocolhood - on its own is not sufficient" ruling (MH is the settling case), + ``docs/source/contributing/conventions/extension-header-subclassing.rst``'s + "own-protocolhood on its own is not sufficient" ruling (MH is the + settling case), that fact *qualifies* an already-standalone header for a base besides :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`; it does not *make* the header standalone, and the RFC has no diff --git a/tests/protocols/internet/test_esp_unit.py b/tests/protocols/internet/test_esp_unit.py index 995420c32..9553bd5d5 100644 --- a/tests/protocols/internet/test_esp_unit.py +++ b/tests/protocols/internet/test_esp_unit.py @@ -1055,8 +1055,9 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: appear after hop-by-hop, routing, and fragmentation extension headers"), not a membership assertion -- membership is the registry's claim, per - ``docs/source/contributing/conventions.rst``'s "being *in* that - registry is what makes something an extension header" ruling. + ``docs/source/contributing/conventions/extension-header-subclassing.rst``'s + "being *in* that registry is what makes something an extension + header" ruling. Rephrasing the placement sentence as "places it among the IPv6 extension headers" overstated the RFC and contradicted this same docstring's ``Note:`` (which says RFC 8200 declines ESP @@ -1069,8 +1070,9 @@ def test_docstring_cites_the_extension_header_registry(self) -> None: also travels directly as an IPv4 payload, exactly as :mod:`~pcapkit.protocols.internet.hip` cites its own IPv4 appendix for the same reason. Per - ``docs/source/contributing/conventions.rst``'s "own-protocolhood - on its own is not sufficient" ruling (MH is the settling case), + ``docs/source/contributing/conventions/extension-header-subclassing.rst``'s + "own-protocolhood on its own is not sufficient" ruling (MH is the + settling case), that fact *qualifies* an already-standalone header for a base besides :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`; it does not *make* the header standalone, and the RFC has no diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 4ff1e6673..801a59585 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -777,7 +777,8 @@ def test_pcapng_option_readers_cover_block_families_and_guards(self) -> None: UnknownOption) from pcapkit.utilities.exceptions import ProtocolError - # Per the mint/unmint ruling for #775 (``docs/source/contributing/conventions.rst``), + # Per the mint/unmint ruling for #775 + # (``docs/source/contributing/conventions/mint-criterion.rst``), # ``Unassigned`` names a procedure rather than a party, so ``FilterType``'s # ``_missing_`` no longer mints a registered ``Unassigned_0`` member for # code 0 -- it returns an unregistered member bearing the bare label @@ -1953,7 +1954,8 @@ def test_pcapng_remaining_constructor_branches_and_custom_dispatch(self) -> None UnknownSecrets as SchemaUnknownSecrets) from pcapkit.utilities.exceptions import ProtocolError - # Per the mint/unmint ruling for #775 (``docs/source/contributing/conventions.rst``), + # Per the mint/unmint ruling for #775 + # (``docs/source/contributing/conventions/mint-criterion.rst``), # ``Unassigned`` names a procedure rather than a party, so ``FilterType``'s # ``_missing_`` no longer mints a registered ``Unassigned_0`` member for # code 0 -- it returns an unregistered member bearing the bare label