From e595d26cfc75eb49676f2a73d5a71b7b24a9ad09 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 1 Oct 2026 10:05:38 -0400 Subject: [PATCH] docs(contributing): sweep the conventions pages for accuracy and concision (#719) Accuracy fixes, each re-derived against the code: - mint-criterion.rst presented `EtherType`'s company names and `Socket`'s `Registered by Xerox` as the worked *mint* examples. #878 converted both to `_unregistered_member`; `CGAType` is now the only `_missing_` that mints, so the criterion decides the unregistered member's *name*, not whether it registers. - Its `ast` snippet matched only `ast.Name` callees, so it reported 1 MINT and 0 UNMINT; `_unregistered_member` is called on `cls`. Matches attributes now. - "Every registry defines `_missing_`" -> 121 of 127, naming the six without one. - registry-protocol.rst: `__new__` exemption said "a handful ... tracked in #860"; it is six named classes and #860 closed with all 127 on the base. - The 6 mh/ngap helpers are not all numeric: `PDUKind` is `str`, and it was listed in two rows at once. - `TCP`/`UDP`/`SCTP`/`DCCP` member *names* come from the service-name column; the values are composites. - process.rst: 9 sections, 8 of them module-level; `pcapkit.interface` has none. - sentinel-convention.rst: `NO_VALUE` also lacks `__copy__`/`__deepcopy__`/ `__reduce__`; the quoted `AbsentType` excerpt did not support the privacy claim it was cited for. - Two `/issues/` links pointed at pull requests (#847, #913). Concision: dropped timed context (the page's former title, the two-pass #877 history, the pre-#937 casing narrative, the pre-restructure changelog shape) and fixed a duplicated clause. Added two Mermaid flows for `_missing_` and for `get`'s dispatch, modelled on workflows.rst:102. Build: docutils parse unchanged from base; tests/project/test_conventions_doc_claims.py and the seven other suites reading these pages 111 passed, 1 skipped, 241 subtests. --- .../extension-header-subclassing.rst | 24 ++-- .../source/contributing/conventions/index.rst | 15 +-- .../conventions/mint-criterion.rst | 102 +++++++++++------ .../contributing/conventions/process.rst | 43 ++++---- .../conventions/registry-protocol.rst | 96 +++++++++------- .../conventions/sentinel-convention.rst | 104 ++++++++---------- 6 files changed, 203 insertions(+), 181 deletions(-) diff --git a/docs/source/contributing/conventions/extension-header-subclassing.rst b/docs/source/contributing/conventions/extension-header-subclassing.rst index 33f235800..5a27a5df6 100644 --- a/docs/source/contributing/conventions/extension-header-subclassing.rst +++ b/docs/source/contributing/conventions/extension-header-subclassing.rst @@ -44,10 +44,10 @@ 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: +**Stated before the criterion itself, because it is the part a future reader will get +wrong.** 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 @@ -96,9 +96,9 @@ 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. +``Shim6`` (140) has the same shape and the same verdict; this package carries no parser +class for it, so nothing implements the classification, but a future one inherits +:class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` alone. .. important:: @@ -139,11 +139,11 @@ was left behind, and that was deliberate. The owner ruled that the ``IPv6_GenericExt`` name goes for good: it was an intermediate state, and it was 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. +What makes that safe rather than merely decided: the old name existed on ``main`` from +``b3551cb63`` to ``93cf940b3``, under four hours on one day, and no release tag falls +between the two commits -- ``git tag --contains`` names the same tags for both -- 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 as a former public name. ESP is an extension header, and still cannot short-circuit ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/source/contributing/conventions/index.rst b/docs/source/contributing/conventions/index.rst index 119c63e00..ba087def9 100644 --- a/docs/source/contributing/conventions/index.rst +++ b/docs/source/contributing/conventions/index.rst @@ -8,13 +8,11 @@ House Conventions 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, and - :ref:`process` the first that governs the repository rather than any of its - code. + :ref:`mint-criterion` and :ref:`registry-protocol` govern + :mod:`pcapkit.const`, which is where the settled questions have mostly + arisen. :ref:`sentinel-convention` governs :mod:`pcapkit.corekit`, + :ref:`extension-header-subclassing` a protocol class hierarchy, and + :ref:`process` the repository rather than any of its code. **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 @@ -22,8 +20,7 @@ House Conventions the page that covers it in the same change that implements it, rather than left in the issue for the next contributor to find. That is the owner's standing ask on - `#918 `__: every - convention settled from here on gets documented here as well. + `#918 `__. .. toctree:: :maxdepth: 1 diff --git a/docs/source/contributing/conventions/mint-criterion.rst b/docs/source/contributing/conventions/mint-criterion.rst index b6a2917dd..00fae5528 100644 --- a/docs/source/contributing/conventions/mint-criterion.rst +++ b/docs/source/contributing/conventions/mint-criterion.rst @@ -3,9 +3,13 @@ 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**: +121 of the 127 registries under :mod:`pcapkit.const` define ``_missing_``, which decides +what happens when a value has no member. The six without one -- +:class:`~pcapkit.const.ipv6.extension_header.ExtensionHeader`, +:class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel` and +:class:`~pcapkit.const.reg.apptype.apptype.AppType`'s four transport subclasses -- carry +no declared-but-unassigned range for one to resolve. 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 @@ -13,67 +17,88 @@ a given range gets is a **design decision, not a style preference**: 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. + Returns a member-like object for the value **without** installing it. The registry + does not grow, and a second lookup of the same value builds an equal but distinct + object rather than handing back the first. + +**Exactly one** ``_missing_`` in the tree mints: +:class:`~pcapkit.const.mh.cga_type.CGAType`'s, whose values are 128-bit CGA extension +tags rather than an IANA-style range of codes. 114 unmint, and the remaining six -- the +five flag registries and :class:`~pcapkit.const.hip.transport.Transport` -- only +range-check and hand back to ``super()._missing_``, declaring no unassigned range at +all. So outside ``CGAType`` the criterion below decides the *name* an unregistered member +carries rather than whether it registers: the registrar's own label suffixed with the +individual code, or the bare block label on its own. The two crawlers that generate the +ranges record that narrowing in their own comments: +:file:`pcapkit/vendor/reg/ethertype.py`'s ``UNASSIGNED_ROW_NAMES`` and +:file:`pcapkit/vendor/ipx/socket.py`'s ``UNASSIGNED_RANGE_NAMES``. + +.. mermaid:: + + flowchart TD + LOOKUP["cls(value) finds no member"] --> MISSING["_missing_"] + MISSING -->|"out of range, or no block covers it"| RAISE["ValueError"] + MISSING -->|"CGAType only"| MINT["extend_enum
permanent member, registry grows"] + MISSING -->|"block label names a party"| SUFFIXED["_unregistered_member
label + _0x<code>"] + MISSING -->|"block label names a procedure"| BARE["_unregistered_member
bare block label"] The criterion ~~~~~~~~~~~~~ The test, paraphrased from the maintainer's ruling: does the upstream registry treat -the label as the final, concrete assigned name (**mint**), or only as a notation for a -human reading the table (**unmint**)? +the label as the final, concrete assigned name, or only as a notation for a human +reading the table? -Settled on `#847 `__ and reaffirmed +Settled on `#847 `__ and reaffirmed on `#775 `__ as a core concept of the ruling. 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 assigns a **real, specific name** to those codes. The name records + something the registry genuinely says, so it is carried across and suffixed with the + code it belongs to. * 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.** + and ``Statically Assigned``. These are written for a human reading the table, so the + bare label stands: suffixing one with a code would **manufacture a name nobody + assigned**, and the value will get its real name if and when something assigns it. Worked examples ~~~~~~~~~~~~~~~ -*Unmint.* :mod:`pcapkit.const.ipx.socket`'s ``Dynamically Assigned``, +*Bare label.* :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``, +*Suffixed.* :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. +range blocks, since four of them hold two blocks each -- and +: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 +Two further ranges in that file carry a suffixed name 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. +and neither is suffixed 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. + code. ``Registered by Xerox`` covers a range and is still suffixed per socket, + because it names *who registered the socket*. ``Dynamically Assigned`` also covers a + range and is 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 -~~~~~~~~~~~~~~~~~~~~~~~~~~ +Why the company names are suffixed +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 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 `__: a +being raised on `#847 `__: a proprietary protocol will never have a public name, so the company name is what serves that purpose in its place. @@ -88,7 +113,10 @@ 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: +not, because ``extend_enum`` also appears in imports and in prose. Match on the +attribute name as well as on the bare name: ``extend_enum`` is called as a plain +function, but ``_unregistered_member`` is called on ``cls``, so a walk that inspects +only ``ast.Name`` nodes finds every mint and no unmint at all: .. code-block:: python @@ -100,8 +128,9 @@ not, because ``extend_enum`` also appears in imports and in prose: 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)} + calls = {n.func.id if isinstance(n.func, ast.Name) else n.func.attr + for n in ast.walk(node) if isinstance(n, ast.Call) + and isinstance(n.func, (ast.Name, ast.Attribute))} if 'extend_enum' in calls: print('MINT ', path) elif '_unregistered_member' in calls: @@ -111,6 +140,7 @@ not, because ``extend_enum`` also appears in imports and in prose: 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. - + finds. :class:`~pcapkit.const.mh.cga_type.CGAType` is the one registry this applies + to today, but snapshot ``{member.value for member in Cls}`` before any lookup + regardless, and use a throwaway process per registry when comparing behaviour across + revisions. diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index ba804bd02..503d81733 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -6,10 +6,7 @@ How the repository itself is run The four pages before this one are about writing library code. The rulings here are about running the repository -- what an install carries, what a changelog entry is, and what the issue and pull request labels mean. None of them is derivable from a -module, which is why they are recorded rather than left to be rediscovered. - -The page exists because those three settled questions fit none of the code-convention -pages, and the owner ruled on +module, and none fits a code-convention page, so the owner ruled on `#918 `__ that they get a page of their own rather than being left in their threads. Each ruling below is **paraphrased rather than quoted**, also on his instruction there; the issue named beside it is @@ -34,7 +31,7 @@ On the tree, in :file:`pyproject.toml`: all = [ "emoji", "cryptography>=3.4", "pycrate" ] So ``all`` is exactly the union of those three extras, written out as literals rather -than referenced -- eight requirements before the ruling, three after it. +than referenced. Two groups stay out, and the reasons are **different** rather than two versions of one reason. Keeping them apart is what stops the list drifting the way it already did @@ -98,19 +95,17 @@ commands down instead of a figure that will be stale by the next merge: # commits on the pull request -- a different number, about a different thing gh pr view 657 -R JarryShaw/PyPCAPKit --json commits -q '.commits|length' -The grouping scheme was settled after that, on #918: **a section per top-level -module, with** ``Added``/``Changed``/``Fixed`` **nested inside each** -- module -granularity, not per-file and not per-subpackage. **That restructure has landed.** It -was a regrouping of scattered entries rather than a transposition of three tidy -blocks: before it, the entries carried their kind as an inline bold label and those -labels alternated in dozens of short stretches, blocked near the top of the file and -thoroughly interleaved below. The file now carries **9** module-level sections holding 155 -entries, and no entry carries an inline kind label any more:: +The grouping scheme was settled on #918: **a section per top-level module, with** +``Added``/``Changed``/``Fixed`` **nested inside each** -- module granularity, not +per-file and not per-subpackage. The file carries **9** module-level sections holding +155 entries, and no entry carries an inline kind label:: $ grep -cE '^\* \*\*(Added|Changed|Fixed)\*\*' docs/source/changelog/1.5.0.rst 0 -The sections are one per top-level module, plus one for what belongs to no module: +Eight of those nine name a module; the ninth, *Project infrastructure*, is for what +belongs to none. Nor is the map one-to-one with the package list below -- +:mod:`pcapkit.interface` has no 1.5.0 entry, so it has no section of its own: .. code-block:: shell @@ -166,10 +161,9 @@ The ones that carry meaning here fall into five groups, which stack rather than compete: a pull request normally carries one from the first group and as many of the rest as apply. **The five groups are not the whole label set** -- the repository also has GitHub's own defaults, of which ``wontfix``, ``invalid``, ``help wanted`` and -``duplicate`` are all in live use -- only ``good first issue`` has never been -applied. They are documented by -GitHub rather than here, and are counted rather than listed so this page does not go -stale every time one is added:: +``duplicate`` are all in live use and only ``good first issue`` has never been applied. +Those are documented by GitHub rather than here, and are counted rather than listed so +this page does not go stale every time one is added:: $ gh label list -R JarryShaw/PyPCAPKit --limit 100 --json name -q '.[].name' | wc -l 29 @@ -215,8 +209,9 @@ commit, so the label and the message agree by construction: values **Issue kind**, for issues rather than pull requests: ``design`` marks a pattern being -decided rather than a defect or a request, which is the label most of the rulings on -these pages were settled under; alongside ``bug``, ``enhancement`` and ``question``. +decided rather than a defect or a request, and a majority of the rulings on these pages +were filed under it -- though not all, several having been settled on a ``bug`` or +``enhancement`` thread instead. Alongside ``bug``, ``enhancement`` and ``question``. **State -- what is happening to it now.** An open issue is meant to carry one of these, so that its status is readable without opening it: @@ -239,7 +234,7 @@ than a category, which is worth checking for rather than assuming away: --json number,labels -q '.[]|"#\(.number) \(.labels|map(.name)|join(","))"' **Review -- the cross-review verdict, at the current head.** Separate from CI, which -has its own status of its own: ``review: pending`` means no verdict for this head, +has a status of its own: ``review: pending`` means no verdict for this head, either never reviewed or the head moved since; ``review: good-to-go`` and ``review: needs-changes`` are the two verdicts. Because they are keyed on the head rather than on the pull request, a new push invalidates the label -- a verdict that @@ -291,9 +286,9 @@ carrying it are that kind: pre-``0.15`` pull requests, ``#3`` to ``#28``, with nothing at all between ``#28`` and ``#350``. Three of those seven are distribution rollups (``#25``, ``#26``, ``#28``, each also carrying ``release``); the other four are early - ``refactor``/``feat`` work from before the project stabilised. On issues it is sparser still, appearing only from - ``#775`` up. So ``breaking``'s absence on an old pull request is weak evidence at - best. The current figures, rather than these: + ``refactor``/``feat`` work from before the project stabilised. On issues it is + sparser still, appearing only from ``#775`` up. So ``breaking``'s absence on an old + pull request is weak evidence at best. The current figures, rather than these: .. code-block:: shell diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index 225ff583c..c73374597 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -17,9 +17,15 @@ Where the registry protocol lives class LinkType(EnumRegistry, IntEnum): ... -A handful of registries define their own ``__new__`` to carry extra attributes and so -do not share the generated template; bringing them onto the base is tracked in -`#860 `__. +Six registries define their own ``__new__`` to carry extra attributes and so do not +share the generated template: :class:`~pcapkit.const.ftp.command.Command`, +:class:`~pcapkit.const.ftp.return_code.ReturnCode`, +:class:`~pcapkit.const.http.method.Method`, +:class:`~pcapkit.const.http.status_code.StatusCode`, +:class:`~pcapkit.const.pcapng.option_type.OptionType` and +:class:`~pcapkit.const.reg.apptype.apptype.AppType`. They are on the base regardless -- +`#860 `__ finished that -- so a +bespoke ``__new__`` exempts a registry from the template, not from the protocol. The Two Tiers, and What Lives on Each ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -44,10 +50,9 @@ registry inherits :class:`~pcapkit.corekit.enum.EnumRegistry` exactly as before. What settled the split is the owner's own second thought about carrying ``register`` on the base: if the base carries ``register``, there is no principled reason for it not to carry ``register_alias`` as well, and the ruling would be the worse for it. -Following that through leaves -:class:`~pcapkit.corekit.enum.EnumRegistry` holding only three methods, too thin to -justify a second class -- so the two tiers collapse into one, which is the opposite of -what was ruled. +Following that through leaves :class:`~pcapkit.corekit.enum.EnumRegistry` holding only +three methods, too thin to justify a second class -- so the two tiers collapse into one, +which is the opposite of what was ruled. **Both tiers are plain classes, and that is load-bearing.** Inserting a parent above :class:`~pcapkit.corekit.enum.EnumRegistry` leaves the member data type exactly where @@ -104,19 +109,6 @@ Three things about it are easy to get wrong: **Zero enumerations remain outside the hierarchy**, measured by the same runtime walk over both the :mod:`enum` and ``aenum`` flavours that once found seven. - It landed in two pull requests rather than one. Seven of the 24 sat in files - other pull requests were editing around the same time: ``CommandType`` and - ``ConformanceRequirement`` in :mod:`pcapkit.const.ftp.command` and its vendor - template, both touched by `#913 `__; - and ``ESPStatus`` in :mod:`pcapkit.protocols.internet.esp` plus all four - :mod:`pcapkit.protocols.internet.mh` helpers (``FastBindingAcknowledgmentStatus``, - ``IPv6AddressPrefixCode``, ``LMAAddressCode``, ``LocalizedRoutingStatus``), both - files touched by `#924 `__. The - first pass (`#921 `__) is - behaviour-preserving on its own, so the other 17 could land without waiting on - those files; the remaining seven followed once both had merged - (`#930 `__). - What a Failed Lookup Raises ~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -143,6 +135,21 @@ So the **provenance** is in-library and the **shape** is stdlib's: ``except ValueError`` around a lookup keeps catching, in this tree and in a caller's. +Which branch a key takes, and what each miss ends in: + +.. mermaid:: + + flowchart TD + GET["get(key, default)"] -->|"isinstance(key, str)"| NAME["_member_map_ lookup"] + GET -->|"otherwise"| VALUE["_validate_value(key)
then cls(key)"] + NAME -->|"hit"| OK["canonical member"] + NAME -->|"miss, key is a registered value"| OK + NAME -->|"miss, default is a registered value"| OK + NAME -->|"miss, no usable default"| KEYERR["EnumKeyError
quiet, derives KeyError"] + VALUE -->|"resolves, or _missing_ answers"| OK + VALUE -->|"ValueError, default is a registered value"| OK + VALUE -->|"ValueError, no usable default"| VALERR["EnumValueError
loud, derives ValueError"] + 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 @@ -203,7 +210,7 @@ guaranteed either: pass a first argument that *is* an instance of the class and delegation **silently succeeds**, so a ``@staticmethod`` override cannot even be relied on to fail loudly. Measured, all three cases, rather than reasoned about. :meth:`~pcapkit.corekit.enum.EnumLookup.get` is itself a ``@classmethod``. The -precedent is `#913 `__, whose +precedent is `#913 `__, whose ``FEATCode.get`` is ``@classmethod def get(cls, key, default=NO_DEFAULT)`` ending in ``return super().get(key, default)``; `#908 `__ followed it, which is @@ -295,7 +302,9 @@ And the reason a registry's spelling is never quietly normalised, from the same an enumeration honours and keeps the original writing the registrar used, and case-insensitivity applies only to the selected registries where it makes logical sense -- ``TransportProtocol`` being one -- or where the RFC documentation itself recognises -the values as case-insensitive, FTP and HTTP commands being the likely candidates. +the values as case-insensitive, FTP and HTTP commands being the candidates he named. +Only FTP survived the audit below: :rfc:`9110#section-9.1` makes the HTTP method token +case-sensitive outright. So :meth:`~pcapkit.corekit.enum.EnumLookup.get` is **case-sensitive**, and that is the default every enumeration gets. Case-insensitivity is a per-class ``get`` override that @@ -445,22 +454,30 @@ That leaves the classes with something to decide: * - ``TCP``, ``UDP``, ``SCTP``, ``DCCP`` - :rfc:`6335#section-5.1` - *"case is ignored for comparison purposes, so both "http" and "HTTP" denote - the same service."* Limb 1, emphatically -- and these registries' **values - are** service names. + the same service."* Limb 1, emphatically -- and these registries **derive every + member name from the registry's service-name column**, in the registrar's own + spelling (``tcpmux``); the *value* is a composite that embeds it + (``'tcpmux [1 - tcp]'``), not the bare name. - **unimplemented** -- no service-name lookup exists to fold; see below * - ``CommandType``, ``ConformanceRequirement`` - :rfc:`959#section-4`, :rfc:`5797#section-2` - Limb 2 holds on measurement: the RFC and registry pages present the kind and conformance letters upper case (``A``/``P``/``S``, ``M``/``O``/``H``) while the CSV columns the crawler reads are lower case in every row (``s`` 26, - ``a`` 18, ``s/p`` 3, blank 1; ``o`` 28, ``m`` 27, ``h`` 7, ``m [1]`` 2). + ``a`` 18, ``p`` 16, ``s/p`` 3, blank 1; ``o`` 28, ``m`` 27, ``h`` 7, + ``m [1]`` 2). Both tallies cover all 64 rows. - **open** -- see below * - The 6 :mod:`~pcapkit.protocols.internet.mh` and :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. **None** of them defines a ``get`` of its own any - more. Two did when this audit was taken -- + - ``FastBindingAcknowledgmentStatus``, ``IPv6AddressPrefixCode``, + ``LMAAddressCode``, ``LocalizedRoutingStatus``, ``Criticality`` and ``PDUKind``. + **Five** of the six carry numeric codes, so the criterion is vacuous on them + exactly as for the :class:`int` tier above; ``PDUKind``'s are :class:`str` ASN.1 + identifiers from 3GPP TS 38.413, and ASN.1 identifiers are case-significant by + construction, so it lands the same way for a different reason. + **None** of the six defines a ``get`` of its own any more. Two did when this + audit was taken -- ``FastBindingAcknowledgmentStatus`` and ``IPv6AddressPrefixCode``, for signature reasons (no ``default``, and an :class:`int`/:class:`str` dispatch) rather than for case -- and @@ -469,7 +486,8 @@ That leaves the classes with something to decide: from :class:`~pcapkit.corekit.enum.EnumLookup` unchanged. ``LMAAddressCode`` and ``LocalizedRoutingStatus`` never carried a ``get`` at all, so they had no string lookup to fold. ``Criticality`` had one when this audit was taken and no - longer does: #921 re-parented it onto + 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. @@ -482,12 +500,10 @@ That leaves the classes with something to decide: - **case-sensitive** -- no override, conforms * - Every other non-registry enumeration - -- - - pcapkit's own discriminators and bit labels, with no registrar behind them - at all -- ``Completion``, ``ftp.Type``, ``httpv1.Type``, ``FinalisedState``, + - All 14 are pcapkit's own discriminators and bit labels, with no registrar behind + them at all -- ``Completion``, ``ftp.Type``, ``httpv1.Type``, ``FinalisedState``, ``ESPStatus``, ``PacketDirection``, ``PacketReception``, and the 7 httpv2 - ``Flags``. ``PDUKind`` is the one with an external source and it points the - same way: its values are ASN.1 identifiers from 3GPP TS 38.413, and ASN.1 - identifiers are case-significant by construction. + ``Flags``. - **case-sensitive** -- nothing to cite, nothing to change Two rows the audit deliberately left open rather than acting on, because each is @@ -495,8 +511,8 @@ wider than a case fix: * **A service-name lookup on the** ``AppType`` **transport registries.** This is the inverse of every other row: :rfc:`6335#section-5.1` *does* make service names - case-insensitive, and ``TCP``/``UDP``/``SCTP``/``DCCP`` hold service names as their - values -- but ``AppType.get`` refuses a non-:class:`int` key, so no service-name + case-insensitive, and ``TCP``/``UDP``/``SCTP``/``DCCP`` take their member names from + that column -- but ``AppType.get`` refuses a non-:class:`int` key, so no service-name lookup exists for the rule to apply to. Implementing one is new public API on a 6,000-member registry where one name maps to many ports, which is a ``get_all`` design question rather than a case fold. @@ -505,10 +521,10 @@ wider than a case fix: specification's own tokens -- the measured spelling disagreement above would make 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. Both classes now inherit - :class:`~pcapkit.corekit.enum.EnumLookup` -- - `#930 `__ finished re-parenting - them, per the note above -- so a ``get`` exists on each, case-sensitive like the + generation time. Both classes inherit + :class:`~pcapkit.corekit.enum.EnumLookup`, since + `#930 `__ completed phase 2's + re-parenting, so a ``get`` exists on each, case-sensitive like the base's own. Whether to fold case to match ``TransportProtocol``'s own override is a design question for whoever writes the first string-keyed caller, not one this audit settles. diff --git a/docs/source/contributing/conventions/sentinel-convention.rst b/docs/source/contributing/conventions/sentinel-convention.rst index 92d7bdb49..541f17b51 100644 --- a/docs/source/contributing/conventions/sentinel-convention.rst +++ b/docs/source/contributing/conventions/sentinel-convention.rst @@ -12,10 +12,9 @@ That is, the class takes the instance's name in CamelCase with ``Type`` appended 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: the owner ruled for SCREAMING_SNAKE and accepted the resulting breaking change outright, with no -backport. 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: +backport. 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 @@ -37,46 +36,24 @@ per-sentinel exception needed. The four in the tree follow it: - ``AbsentType`` - :mod:`pcapkit.corekit.sentinels` -All four used to live beside the one class that used them -- +GitHub issue #911's housing ruling -- one module for all four -- is why the table names +a single defining module. Each of the four modules that *uses* a sentinel keeps a +re-export of it, so ``from import `` keeps working for :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 -- one module for all four -- 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 owner ruled on #937 that dropping it is fine, so -long as the documentation states that the type and class are private and not for public -use -- that alone 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. +:mod:`pcapkit.corekit.enum` and :mod:`pcapkit.protocols.protocol` alike, including a +caller's ``if TYPE_CHECKING:``-only import of the type. + +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. + +``ABSENT`` carries no leading underscore even though it is private -- it is read in +``_declared_keywords`` and discarded there, never leaving +:mod:`pcapkit.protocols.protocol`. The owner ruled on #937 that dropping the underscore +is fine so long as the documentation states that the type and the object are private and +not for public use, which is what this page and +:class:`~pcapkit.corekit.sentinels.AbsentType`'s own docstring do in its place. So +**privacy here is documentation-only**, and nothing in the name marks it out: when +adding a sentinel, add it to the table above whether or not it is public. What reaches users is the **object only**. The owner ruled on GitHub issue #911 that the objects alone -- ``NULL`` and its siblings -- are exported to users, so a public @@ -92,11 +69,11 @@ 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. + example. :class:`~pcapkit.corekit.sentinels.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()`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -126,8 +103,8 @@ appears in a signature, in :func:`help` output and in a traceback. Compare what 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. +house-style rewrite destroys that in exchange for a sentinel nobody outside the backport +ever sees. The rule above is for sentinels this package writes itself. What to implement, and what not to ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -142,10 +119,9 @@ inconsistencies**: 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. + code holding the pre-reload instance will fail ``is``. The module that matters there + is :mod:`pcapkit.corekit.sentinels`, where the class statement lives; reloading one + of the re-exporting modules has no effect on the sentinel. ``__bool__`` returning :obj:`False` ``NULL``, ``NO_VALUE`` and ``ABSENT`` have it, because each stands for an *absent @@ -157,12 +133,20 @@ inconsistencies**: prevent. ``__copy__`` / ``__deepcopy__`` / ``__reduce__`` - :class:`~pcapkit.corekit.sentinels.NullType` has them because ``NULL`` is stored in a + :class:`~pcapkit.corekit.sentinels.NullType` is the only one of the four that 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. + reconstruct a second instance. ``NO_VALUE``, ``NO_DEFAULT`` and ``ABSENT`` have none. + Measured, since the consequence differs: ``NO_DEFAULT`` survives a copy identically + anyway, because its ``__new__`` hands back the cached instance, while + ``copy.copy(NO_VALUE)`` and ``copy.copy(ABSENT)`` each build a distinct object that + fails both ``is`` and ``==``. Neither is reached by a copy that would notice: + ``ABSENT`` never leaves the + module that reads it, and ``NO_VALUE``, though it rides + :attr:`FieldBase.default ` through a + field copy once per field per packet, keeps its identity there because + :meth:`FieldBase.__copy__ ` is + shallow and shares the reference. Add all three when, and only when, the sentinel + becomes reachable from a copy that is not.