From 31a0adc901f03fd62f91acca140768c476814f2b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 2 Oct 2026 12:33:58 -0400 Subject: [PATCH 1/2] docs(protocols): state the rulings behind mh, vendor and exceptions in our own words Refs #987, #719. - mh.py: four docstring passages quoted the maintainer; restate the ruling on #935 (delete both get overrides rather than widen them) and what it bought. - vendor/__main__.py: restate the #872 snapshot-and-revert ruling and the keep-zero-exited-targets rule instead of quoting them. - utilities/exceptions.py: restate the stdlib-shaped exception ruling carried out by #923. Docstrings only; token streams identical with string literals masked. --- pcapkit/protocols/internet/mh.py | 40 +++++++++++++++++++------------- pcapkit/utilities/exceptions.py | 8 +++---- pcapkit/vendor/__main__.py | 13 ++++++----- 3 files changed, 35 insertions(+), 26 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 23651ebfb..8ff74130a 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -581,11 +581,16 @@ class FastBindingAcknowledgmentStatus(EnumLookup, IntEnum): This class carried its own hand-rolled ``get()`` override through #930, and briefly again through GitHub issue #935's first attempt, which widened the override to accept ``default`` rather than delete - it outright. A ruling given in review of the work for #935 went the - other way, verbatim -- asked *"why must we have the two overrides - tho? cant they directly fall back to the base class's?"*, the answer - was *"I prefer (2) directly"*, ``(2)`` naming deletion among the - ruling's own options. Measured before acting on it: the override's own + it outright. A ruling given in review of the work for #935 first + leaned toward that widening, then went the other way: delete both + ``get`` overrides in this module rather than widen them, so + :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode` inherit + :meth:`~pcapkit.corekit.enum.EnumLookup.get` outright. That also + removes the ``# type: ignore[override]`` suppressions the overrides + needed, and makes all seven re-parented classes behave alike on + ``get``, which had not been literally true. Measured before acting + on it: the override's own docstring called it a "Backport support for original codes", but this class mints no alias -- ``__members__`` and ``list(cls)`` agree at 6 -- so what the override actually did was resolve an @@ -698,11 +703,16 @@ class IPv6AddressPrefixCode(EnumLookup, IntEnum): This class carried its own hand-rolled ``get()`` override through #930, and briefly again through GitHub issue #935's first attempt, which widened the override to accept ``default`` rather than delete - it outright. A ruling given in review of the work for #935 went the - other way, verbatim -- asked *"why must we have the two overrides - tho? cant they directly fall back to the base class's?"*, the answer - was *"I prefer (2) directly"*, ``(2)`` naming deletion among the - ruling's own options. Measured before acting on it: the override's own + it outright. A ruling given in review of the work for #935 first + leaned toward that widening, then went the other way: delete both + ``get`` overrides in this module rather than widen them, so + :class:`FastBindingAcknowledgmentStatus` and + :class:`IPv6AddressPrefixCode` inherit + :meth:`~pcapkit.corekit.enum.EnumLookup.get` outright. That also + removes the ``# type: ignore[override]`` suppressions the overrides + needed, and makes all seven re-parented classes behave alike on + ``get``, which had not been literally true. Measured before acting + on it: the override's own docstring called it a "Backport support for original codes", but this class mints no alias -- ``__members__`` and ``list(cls)`` agree at 4 -- so what the override actually did was resolve an @@ -839,9 +849,8 @@ class LocalizedRoutingStatus(EnumLookup, IntEnum): -- tests included -- so GitHub issue #880 deleted it outright rather than rebuilding it on the immutable contract, the same conclusion #935 reached separately for the other two, on a ruling given in - review of that work, verbatim: *"I prefer (2) directly"* -- ``(2)`` - being deletion of those two overrides rather than widening them to - match the base. + review of that work: delete those two overrides rather than widen + them to match the base, as an earlier lean on the issue had it. GitHub issue #930's re-parenting above gives this class ``get``/``get_all`` again, but as the base's own bare lookup rather than a bespoke override -- it still cannot mint, so an unassigned @@ -919,9 +928,8 @@ class LMAAddressCode(EnumLookup, IntEnum): -- tests included -- so GitHub issue #880 deleted it outright rather than rebuilding it on the immutable contract, the same conclusion #935 reached separately for the other two, on a ruling given in - review of that work, verbatim: *"I prefer (2) directly"* -- ``(2)`` - being deletion of those two overrides rather than widening them to - match the base. + review of that work: delete those two overrides rather than widen + them to match the base, as an earlier lean on the issue had it. GitHub issue #930's re-parenting above gives this class ``get``/``get_all`` again, but as the base's own bare lookup rather than a bespoke override -- it still cannot mint, so an unassigned diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index be86a13b3..50f980c02 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -716,10 +716,10 @@ class EnumKeyError(BaseError, KeyError): taste: ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises :exc:`ValueError`, so a lookup that misses by *name* is :exc:`KeyError`-derived and one that misses by *value* is - :exc:`ValueError`-derived. A ruling recorded on GitHub issue #923, - verbatim: *"Either ``ValueError`` or ``KeyError``, that's depending on how - stdlib's ``Enum`` would raise on these circumstances. And we should raise - one from ``pcapkit.utilities.exceptions`` rather builtin exceptions."* + :exc:`ValueError`-derived. That is a ruling given in review of the + re-parenting work for #877, carried out by GitHub issue #923: raise + whichever of the two stdlib :class:`~enum.Enum` would raise in the same + circumstances, and raise it from this module rather than as a builtin. Deriving from :exc:`KeyError` is what makes that ruling cheap to carry out: :meth:`~pcapkit.corekit.enum.EnumLookup.get` raised a bare builtin diff --git a/pcapkit/vendor/__main__.py b/pcapkit/vendor/__main__.py index 92eb770e4..76f2b5f59 100644 --- a/pcapkit/vendor/__main__.py +++ b/pcapkit/vendor/__main__.py @@ -51,12 +51,13 @@ def get_parser() -> 'ArgumentParser': def _snapshot_and_restore(vendor: 'Type[Vendor]') -> 'Iterator[None]': """Copy a target's const file aside before it runs; restore it if it raises. - A ruling given in review of the work for #872, verbatim: *"an easier - path is simply keep a copy before running the sub-vendor and revert if - anything failed."* This is that -- at the per-target boundary - :func:`run` already owns, which is also exactly where the ruling's - ``(b)``, "only discard changes made by a non-zero sub-vendor", wants - the discarding to happen. + A ruling given in review of the work for #872 settled how a failed target + is undone: keep a copy of its const file before running the sub-vendor and + revert if anything failed, rather than making the write itself atomic. + This is that -- at the per-target boundary :func:`run` already owns, which + is also exactly where the earlier ruling on the same work wants the + discarding to happen: only a non-zero sub-vendor's changes are discarded, + and a zero-exited one's are kept. It is a *wider* guarantee than protecting the single ``open``/``print`` pair :meth:`~pcapkit.vendor.default.Vendor.__init__` From ba49e0f94220d595fc61a7e67cc7daa2b45d1aa1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 2 Oct 2026 13:05:43 -0400 Subject: [PATCH 2/2] docs(protocols): attribute the get-override ruling to where it was given The cross-review on #991 found mh.py placing the lean toward widening in a review that did not exist yet. Verified against the API: - the lean is comment 5901325827 on issue #935, 2026-09-29T23:57:26Z, given on the issue itself rather than in any review; - the reversal is comment 5902590532 on the widening attempt, 2026-09-30 T02:00:58Z; - that attempt's earliest comment of any kind is 2026-09-30T01:45:20Z, so the lean predates it by 1h47m48s and is what caused it to be opened. The passage now reads as an earlier lean on the issue, overtaken by a later ruling in review of the attempt, matching the form this file already used 260 lines further down. - The trailing clause at the two sibling sites could attach to either verb, and only the widening was ever preferred; reworded so it cannot invert. - exceptions.py named the #877 re-parenting adjacent to the issue that did not carry it out. #877's phase 2 is the half the ruling was reviewed on. - vendor/__main__.py cross-referenced Vendor._write_atomic with :meth:, which resolves to nothing: git grep finds the method nowhere under pcapkit/vendor/ on this branch or on main, and the same sentence says it was deleted. Now a plain literal. - Re-flowed the five touched paragraphs; remaining short lines are forced by a following role too long to fit. Prose only: masked-token sequences identical in all three files, every differing string token is a docstring, and the AST with docstrings blanked compares equal. Maximum line length unchanged at 190/99/102. Tests: 52 passed / 499 subtests, and 121 passed / 104 subtests. --- pcapkit/protocols/internet/mh.py | 69 +++++++++++++++----------------- pcapkit/utilities/exceptions.py | 4 +- pcapkit/vendor/__main__.py | 23 +++++------ 3 files changed, 46 insertions(+), 50 deletions(-) diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 8ff74130a..fd07b6a2f 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -581,30 +581,28 @@ class FastBindingAcknowledgmentStatus(EnumLookup, IntEnum): This class carried its own hand-rolled ``get()`` override through #930, and briefly again through GitHub issue #935's first attempt, which widened the override to accept ``default`` rather than delete - it outright. A ruling given in review of the work for #935 first - leaned toward that widening, then went the other way: delete both - ``get`` overrides in this module rather than widen them, so - :class:`FastBindingAcknowledgmentStatus` and + it outright. An earlier lean on issue #935 had preferred that + widening; a later ruling in review of the attempt went the other + way: delete both ``get`` overrides in this module rather than widen + them, so :class:`FastBindingAcknowledgmentStatus` and :class:`IPv6AddressPrefixCode` inherit :meth:`~pcapkit.corekit.enum.EnumLookup.get` outright. That also removes the ``# type: ignore[override]`` suppressions the overrides needed, and makes all seven re-parented classes behave alike on ``get``, which had not been literally true. Measured before acting - on it: the override's own - docstring called it a "Backport support for original codes", but - this class mints no alias -- ``__members__`` and ``list(cls)`` - agree at 6 -- so what the override actually did was resolve an - :class:`int` by direct construction and a name by subscript, - exactly the dual resolution + on it: the override's own docstring called it a "Backport support + for original codes", but this class mints no alias -- + ``__members__`` and ``list(cls)`` agree at 6 -- so what the override + actually did was resolve an :class:`int` by direct construction and + a name by subscript, exactly the dual resolution :meth:`~pcapkit.corekit.enum.EnumLookup.get` already provides for every other :class:`int`-valued registry in this tree. There was - nothing left to backport. ``get``/``get_all`` now come from the - base alone, the same as the five other re-parents #930 finished - alongside this one -- including :class:`LocalizedRoutingStatus` - and :class:`LMAAddressCode` below, whose own hand-rolled ``get()`` - GitHub issue #880 had already deleted outright, for the same - reason: zero callers depended on anything the base does not - already do. + nothing left to backport. ``get``/``get_all`` now come from the base + alone, the same as the five other re-parents #930 finished alongside + this one -- including :class:`LocalizedRoutingStatus` and + :class:`LMAAddressCode` below, whose own hand-rolled ``get()`` + GitHub issue #880 had already deleted outright, for the same reason: + zero callers depended on anything the base does not already do. A behaviour change comes with the deletion, deliberately: the override branched on ``isinstance(key, int)`` and routed every @@ -703,31 +701,30 @@ class IPv6AddressPrefixCode(EnumLookup, IntEnum): This class carried its own hand-rolled ``get()`` override through #930, and briefly again through GitHub issue #935's first attempt, which widened the override to accept ``default`` rather than delete - it outright. A ruling given in review of the work for #935 first - leaned toward that widening, then went the other way: delete both - ``get`` overrides in this module rather than widen them, so - :class:`FastBindingAcknowledgmentStatus` and + it outright. An earlier lean on issue #935 had preferred that + widening; a later ruling in review of the attempt went the other + way: delete both ``get`` overrides in this module rather than widen + them, so :class:`FastBindingAcknowledgmentStatus` and :class:`IPv6AddressPrefixCode` inherit :meth:`~pcapkit.corekit.enum.EnumLookup.get` outright. That also removes the ``# type: ignore[override]`` suppressions the overrides needed, and makes all seven re-parented classes behave alike on ``get``, which had not been literally true. Measured before acting - on it: the override's own - docstring called it a "Backport support for original codes", but - this class mints no alias -- ``__members__`` and ``list(cls)`` - agree at 4 -- so what the override actually did was resolve an - :class:`int` by direct construction and a name by subscript, - exactly the dual resolution + on it: the override's own docstring called it a "Backport support + for original codes", but this class mints no alias -- + ``__members__`` and ``list(cls)`` agree at 4 -- so what the override + actually did was resolve an :class:`int` by direct construction and + a name by subscript, exactly the dual resolution :meth:`~pcapkit.corekit.enum.EnumLookup.get` already provides for every other :class:`int`-valued registry in this tree. There was - nothing left to backport. ``get``/``get_all`` now come from the - base alone, the same as the five other re-parents #930 finished - alongside this one -- including + nothing left to backport. ``get``/``get_all`` now come from the base + alone, the same as the five other re-parents #930 finished alongside + this one -- including :class:`~pcapkit.protocols.internet.mh.LocalizedRoutingStatus` and :class:`~pcapkit.protocols.internet.mh.LMAAddressCode` below, whose own hand-rolled ``get()`` GitHub issue #880 had already deleted - outright, for the same reason: zero callers depended on anything - the base does not already do. + outright, for the same reason: zero callers depended on anything the + base does not already do. A behaviour change comes with the deletion, deliberately: the override branched on ``isinstance(key, int)`` and routed every @@ -850,8 +847,8 @@ class LocalizedRoutingStatus(EnumLookup, IntEnum): than rebuilding it on the immutable contract, the same conclusion #935 reached separately for the other two, on a ruling given in review of that work: delete those two overrides rather than widen - them to match the base, as an earlier lean on the issue had it. - GitHub issue #930's re-parenting above gives this class + them to match the base, which an earlier lean on the issue had + preferred. GitHub issue #930's re-parenting above gives this class ``get``/``get_all`` again, but as the base's own bare lookup rather than a bespoke override -- it still cannot mint, so an unassigned value raises through ``get`` exactly as it does through the bare @@ -929,8 +926,8 @@ class LMAAddressCode(EnumLookup, IntEnum): than rebuilding it on the immutable contract, the same conclusion #935 reached separately for the other two, on a ruling given in review of that work: delete those two overrides rather than widen - them to match the base, as an earlier lean on the issue had it. - GitHub issue #930's re-parenting above gives this class + them to match the base, which an earlier lean on the issue had + preferred. GitHub issue #930's re-parenting above gives this class ``get``/``get_all`` again, but as the base's own bare lookup rather than a bespoke override -- it still cannot mint, so an unassigned value raises through ``get`` exactly as it does through the bare diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 50f980c02..d91d8a3fd 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -716,8 +716,8 @@ class EnumKeyError(BaseError, KeyError): taste: ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises :exc:`ValueError`, so a lookup that misses by *name* is :exc:`KeyError`-derived and one that misses by *value* is - :exc:`ValueError`-derived. That is a ruling given in review of the - re-parenting work for #877, carried out by GitHub issue #923: raise + :exc:`ValueError`-derived. That is a ruling given in review of #877's + phase-2 re-parenting, carried out by GitHub issue #923: raise whichever of the two stdlib :class:`~enum.Enum` would raise in the same circumstances, and raise it from this module rather than as a builtin. diff --git a/pcapkit/vendor/__main__.py b/pcapkit/vendor/__main__.py index 76f2b5f59..87beac897 100644 --- a/pcapkit/vendor/__main__.py +++ b/pcapkit/vendor/__main__.py @@ -102,18 +102,17 @@ def _snapshot_and_restore(vendor: 'Type[Vendor]') -> 'Iterator[None]': A symlinked destination has a related divergence from the pre-#872 behaviour -- documented against the write path by rounds 4-8 (deleted - along with :meth:`~pcapkit.vendor.default.Vendor._write_atomic`, though - the underlying behaviour persists here instead): ``open(const_file, - 'w')`` writes *through* a symlink, into whatever file it points at, - leaving the link itself untouched. ``os.replace(backup, const_file)`` - on restore instead replaces the *link itself* with a regular file. - Measured, for ``link.py`` symlinked to ``real.py``: after a failure and - restore, ``real.py`` is left however the crawler's failed write left - it (truncated, in this case -- nothing here restores the file a - symlink used to point at), ``link.py`` is now a regular file holding - the backup's content, and ``os.path.islink(link.py)`` is - :data:`False`. ``find pcapkit/const -type l`` is still empty, so this - remains latent. + along with ``_write_atomic``, though the underlying behaviour persists + here instead): ``open(const_file, 'w')`` writes *through* a symlink, + into whatever file it points at, leaving the link itself untouched. + ``os.replace(backup, const_file)`` on restore instead replaces the *link + itself* with a regular file. Measured, for ``link.py`` symlinked to + ``real.py``: after a failure and restore, ``real.py`` is left however + the crawler's failed write left it (truncated, in this case -- nothing + here restores the file a symlink used to point at), ``link.py`` is now a + regular file holding the backup's content, and + ``os.path.islink(link.py)`` is :data:`False`. ``find pcapkit/const + -type l`` is still empty, so this remains latent. Nothing is copied at all when the destination does not exist yet: there is no previous file for a failure to discard, so there is nothing this