diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 818b0d01ba..9bdf2528c6 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -51,7 +51,7 @@ name: "Lint" # have been deleted, not because the code changed. Already superseded the # same day it was measured, too: a cross-review measured `R0801 # duplicate-code` alone moving 400 -> 573 on `merge(f31e5114f, 83b58ebda)` -- -# this branch merged onto the `main` it was rebased against before #769 +# this branch merged onto the `main` it was rebased against before #768 # landed there -- taking R to 823 and the total to 6220 on that merge. Still # true of `main` itself today, since this PR touches no `pcapkit/` file, but # named for the tree it was actually measured on rather than asserted of @@ -62,7 +62,7 @@ name: "Lint" # actually unchanged between them, despite what an earlier revision of this # comment claimed: four files moved (`pcapkit/const/reg/apptype/apptype.py`, # `pcapkit/foundation/extraction.py`, `pcapkit/protocols/schema/schema.py`, -# `pcapkit/vendor/reg/apptype/apptype.py`; tree 7270ad50 -> 2b9ac808), #764's +# `pcapkit/vendor/reg/apptype/apptype.py`; tree 7270ad50 -> 2b9ac808), #758's # port-validation logic among them. E, W and C read identically either side of # that diff -- 90/4765/542 both times -- but 1a852698b is the tree actually # re-measured for this line, so that is what it is pinned to now, rather than diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 7d89f24bf7..74a42246cc 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -96,7 +96,7 @@ jobs: # `python_version < '3.12'` marker lets it install), installing it turned # 7 of the 10 HAS_PYPCAPFILE-gated methods this comment used to count # into failures, against a real bug in pcapkit/toolkit/pypcapfile.py's - # handling of pypcapfile 0.12.0's `IP.src`/`IP.dst`. #747 and #748 fixed + # handling of pypcapfile 0.12.0's `IP.src`/`IP.dst`. #743 and #746 fixed # that bug, and #751 gave the now-safe extra a home: the dedicated # `engine-tests` and `pypcap-parity` jobs below install it (15 # HAS_PYPCAPFILE-gated methods between them), deliberately apart from @@ -169,7 +169,7 @@ jobs: # PyPCAPFile is NOT added here either, even though this job's selection # also reaches test_new_engine_parity_runtime.py's 6 HAS_PYPCAPFILE # methods -- see the `test` job's comment above for the bug that used to - # make installing it here a regression, now fixed by #747/#748. #751's + # make installing it here a regression, now fixed by #743/#746. #751's # `pypcap-parity` job below covers those 6 methods instead, on the same # fixture-tier selection as this job but in a venv of its own. - name: Install package, test and generator dependencies @@ -251,7 +251,7 @@ jobs: # jobs is textually part of the one above it, and that guard's pytest_jobs() # requires exactly one such literal per job section -- a second one here, # even in a comment, makes it think this job has two install lines and - # cannot tell which extras the run would have. See #849's cross-review.), + # cannot tell which extras the run would have. See #845's cross-review.), # so "Engines Python 3.12/3.13/3.14" reported # green while never exercising PyPCAPFile at all on three of five legs -- # yet all three were promoted to ruleset 23497679's required checks anyway @@ -482,7 +482,7 @@ jobs: # comment here would make that guard think this section holds two # baseline install lines), and running them unchecked used to mean a # broken base install on a `not-installable` cell read as "expected" - # while the job had installed and run nothing at all (#849's + # while the job had installed and run nothing at all (#845's # cross-review). They now run under this step's own explicit `set -eu` # below -- not "the shell's default", since GitHub's default for a # Linux `run:` step is plain `bash -e {0}` with no `pipefail`, and @@ -546,7 +546,7 @@ jobs: # bare "5xx or 429" number scan: pip prints a "(NNN kB)" download # size on essentially every sdist fetch, and a Cython-generated # .c file runs to thousands of lines, so an unanchored bare-number - # match (#849's cross-review) fires on ordinary download-size + # match (#845's cross-review) fires on ordinary download-size # lines and on compiler diagnostics quoting a line number in that # range -- e.g. "Downloading pypcap-1.3.0.tar.gz (500 kB)" and # "pcap.c:501:24: error: ..." both matched, while the actual @@ -596,8 +596,9 @@ jobs: # those files themselves assert once they see this interpreter's # version, e.g. PyPCAPFile's own # test_unsupported_reason_tracks_the_running_interpreter and PyShark's - # test_the_reason_tracks_the_running_interpreter (added in #846) both - # branch on sys.version_info against the engine's own PYTHON_CEILING. + # test_the_reason_tracks_the_running_interpreter (added for PyShark's + # real-capture coverage) both branch on sys.version_info against the + # engine's own PYTHON_CEILING. # That is what proves the decline actually fired and named the # interpreter, rather than this job asserting it from the outside. # Skipped entirely for `not-installable` cells -- see the install step. @@ -958,7 +959,7 @@ jobs: # above, which each cover only the subset their own selection reaches. # PyPCAPFile is deliberately NOT added here -- see the `test` job's # comment above for the bug that used to make installing it a - # regression, now fixed by #747/#748. This job runs on 3.14 only, where + # regression, now fixed by #743/#746. This job runs on 3.14 only, where # PyPCAPFile's marker resolves to nothing regardless, so adding it here # would be a no-op that misleadingly suggests the extra is exercised by # `gate`; #751's `engine-tests` and `pypcap-parity` jobs cover it for @@ -1129,7 +1130,7 @@ jobs: # keeps that visibility, but means duplicating this job's system-package # install, per-engine install logic, and test invocation -- measured at # 182 raw lines across those three steps, and GitHub Actions has no YAML - # anchors to shrink that with -- into a second job immediately after #849 + # anchors to shrink that with -- into a second job immediately after #845 # rebuilt this one, to close a single unverified edge. Flagging the risk # here instead: if a `required-checks` run ever goes red with only a 3.15 # `engine-tests` leg failing underneath it, that is this exact gap and not diff --git a/pcapkit/const/ftp/command.py b/pcapkit/const/ftp/command.py index a8d538d200..7cd18554b0 100644 --- a/pcapkit/const/ftp/command.py +++ b/pcapkit/const/ftp/command.py @@ -35,7 +35,7 @@ class FEATCode(EnumRegistry, StrEnum): :meth:`~pcapkit.vendor.ftp.command.Command.process`) -- rather than minting the per-command ones at import time as an incidental side effect of building :class:`Command`'s own rows. GitHub issue #860: - that import-time mutation was the same defect shape #861 removed + that import-time mutation was the same defect shape #775 removed from :class:`~pcapkit.const.pcapng.filter_type.FilterType`, just not previously noticed here. ``_missing_`` still unmints for a keyword that turns up on the wire but names none of these -- the diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index 54bc17477c..80884bc76f 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -43,12 +43,12 @@ class TransportProtocol(EnumLookup, IntEnum): # ``undefined`` is declared explicitly as ``0`` and every other member # is ``auto()``, which continues from the preceding explicit value # rather than needing its own ``_start_ = 0`` to begin there; the - # owner's own ruling on this issue (#860) is explicit that + # a ruling given in review of the work for #860 is explicit that # ``undefined`` stays a direct ``0`` for that reason: "undefined # direct uses 0. then other real transport use auto. so we don't have # to define a _start_ and the undefined declaration is explicit." - # GitHub issue #808 already dropped the ``IntFlag`` base once - # nothing built a composite, and GitHub PR #836's ruling later + # GitHub issue #808 already dropped the ``IntFlag`` base once nothing + # built a composite, and a ruling given in review of that work later # retired ``|``-composite decoding entirely: "since it's no longer a # Flag, `|` joined values are no longer parsed and accepted, we will # treat it as a whole, instead of splitting." With no decoding left to @@ -61,7 +61,7 @@ class TransportProtocol(EnumLookup, IntEnum): # bare -- into anything narrower than the whole value it already is, on # either numbering; a plain ``dict.get`` lookup cannot tell a composed # ``int`` from any other one that happens to equal it. Treating every - # int as a whole is exactly what #836's ruling above asks for, and it + # int as a whole is exactly what that ruling above asks for, and it # is also why this renumbering changes what specific ints mean, not # only what composed ones do: # ``tcp | udp`` (``3``) used to name no registry and now resolves as @@ -108,22 +108,23 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco on these circumstances."* A stdlib ``E['nosuch']`` raises :exc:`KeyError`, and #923's census of the 127 concrete :class:`~pcapkit.corekit.enum.EnumLookup` subclasses -- taken before - #921 re-parented this class, so this class is not among them -- found - 119 already answering a name miss that way against 5 answering with - :exc:`ValueError`. Those 5 are :class:`AppType` and its four transport - registries, and they land there only because their own ``get()`` takes - an :class:`int` port and never accepts a name at all, rather than from - any name-miss policy. So there was no policy here to preserve, and a - name miss now reaches the caller as + the phase-2 re-parenting moved this class, so it is not among them -- + found 119 already answering a name miss that way against 5 answering + with :exc:`ValueError`. Those 5 are :class:`AppType` and its four + transport registries, and they land there only because their own + ``get()`` takes an :class:`int` port and never accepts a name at all, + rather than from any name-miss policy. So there was no policy here + to preserve, and a name miss now reaches the caller as :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from the base. - Maintainer ruling on GitHub PR #836 -- "Do not allow extension of - TransportProtocol at all" -- is untouched by that: the refusal is still - a refusal and still mints nothing, only its exception class moved. + Maintainer ruling given in review of the work for #808 -- "Do not + allow extension of TransportProtocol at all" -- is untouched by + that: the refusal is still a refusal and still mints nothing, only + its exception class moved. The base is a :class:`classmethod` (:meth:`~pcapkit.corekit.enum.EnumLookup.get`), so this override moves from :class:`staticmethod` to :class:`classmethod` to - delegate at all -- the same move GitHub issue #908 and #915 made for + delegate at all -- the same move GitHub issue #908 made for :meth:`~pcapkit.const.http.method.Method.get`. Grepped every call site in this tree for GitHub issue #877: all call this method by name, none take it as a bare callable or introspect ``__func__``, @@ -163,19 +164,20 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco """ if isinstance(key, str): return super().get(key.lower(), default) - # NOTE: maintainer ruling on this PR (#836): "Do not allow extension - # of TransportProtocol at all." A name that is not a declared member - # used to mint a brand-new one here, at ``max_val + 1`` (before that, - # ``max_val * 2``) -- an unbounded, ever-growing set of transport - # protocols nothing ever asked for. There is nothing left to walk - # now: it is simply refused, exactly like any other unrecognised - # name -- including one spelling a composite, e.g. ``'tcp|udp'``. + # NOTE: maintainer ruling given in review of the work for #808: "Do + # not allow extension of TransportProtocol at all." A name that is + # not a declared member used to mint a brand-new one here, at + # ``max_val + 1`` (before that, ``max_val * 2``) -- an unbounded, + # ever-growing set of transport protocols nothing ever asked for. + # There is nothing left to walk now: it is simply refused, exactly + # like any other unrecognised name -- including one spelling a + # composite, e.g. ``'tcp|udp'``. # ``'|'`` used to be intercepted here on its own, so a composite in # disguise never got minted into a member whose own name lied about - # being a single transport; the owner's further ruling on this PR - # retired that special case along with the rest of the composite - # handling once TransportProtocol stopped being a Flag at all: - # "since it's no longer a Flag, `|` joined values are no longer + # being a single transport; a further ruling given in review of the + # work for #808 retired that special case along with the rest of the + # composite handling once TransportProtocol stopped being a Flag at + # all: "since it's no longer a Flag, `|` joined values are no longer # parsed and accepted, we will treat it as a whole, instead of # splitting." A ``'|'``-joined name is therefore not special any # more -- it is simply not the name of a declared member, and gets @@ -2524,10 +2526,10 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ the same as any caller passing a literal port-transport bitmask -- falls through to ``int.__or__`` and returns a bare :class:`int` rather than a member. Never split back into the - transports its bits would each name -- owner ruling on this PR - (#836) -- so it is looked up as the one whole value it - already is, exactly like any other bare int: refused when - that whole value names no registry, resolved when it + transports its bits would each name -- a ruling given in review + of the work for #808 -- so it is looked up as the one whole + value it already is, exactly like any other bare int: refused + when that whole value names no registry, resolved when it happens to equal one instead. Since GitHub issue #860 moved this class off power-of-two spacing, a hand-built composite is no longer guaranteed to be the former -- see @@ -2575,11 +2577,12 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ # either numbering. A genuine member reaching this point is # ``undefined`` -- the four real transports would already have # resolved above, and :meth:`TransportProtocol.get` cannot mint - # anything else, per this PR's own maintainer ruling against - # extending TransportProtocol at all -- and a bare :class:`int` - # reaches here whenever it matches no real member's value, e.g. a - # stray bit like ``17``. A composite built by hand used to reach - # here just as reliably, since no combination of the old power-of- + # anything else, per the maintainer ruling given in review of the + # work for #808, against extending TransportProtocol at all -- and + # a bare :class:`int` reaches here whenever it matches no real + # member's value, e.g. a stray bit like ``17``. A composite built by + # hand used to reach here just as reliably, since no combination of + # the old power-of- # two bits ever equalled a single real member's value; that is no # longer true under this class's current sequential numbering -- # ``TransportProtocol.tcp | TransportProtocol.udp`` (``3``) is @@ -2590,10 +2593,10 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ # and naming every transport whose bit was set -- the fix for # GitHub issue #759, where resolving a composite by picking its # lowest set bit dispatched every one containing ``tcp`` into the - # TCP registry regardless of what else it named. The owner's - # further ruling on this PR (#836) retired that decoding along with - # the rest of the composite handling: "since it's no longer a Flag, - # `|` joined values are no longer parsed and accepted, we will + # TCP registry regardless of what else it named. A further ruling + # given in review of the work for #808 retired that decoding along + # with the rest of the composite handling: "since it's no longer a + # Flag, `|` joined values are no longer parsed and accepted, we will # treat it as a whole, instead of splitting." So a composite's bits # are never decoded looking for a partial answer any more, on # either numbering -- it is looked up as the one whole value it diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 7f4628ccbd..276e871375 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -47,8 +47,9 @@ the base above was deliberately behaviour-preserving on its own, so that it could land while other work was still in flight on the files the re-parent touches, and the phase itself landed in two pull requests for exactly that - reason -- #921 for the 17 enumerations that were free to move at once, and - `#930 `__ for the + reason -- the first for the 17 enumerations that were free to move at + once, and the second, `#930 + `__, for the remaining seven once the files holding them freed up. The registry tier's own shape is the earlier ruling on GitHub issue #842, @@ -340,12 +341,13 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': on the same terms; the ``str`` path does not call the hook, for the reason given on :meth:`_validate_value` itself. - Both failure paths raise from :mod:`pcapkit.utilities.exceptions` rather - than a builtin, per the owner's ruling on GitHub issue #923: *"Either - ``ValueError`` or ``KeyError``, that's depending on how stdlib's - ``Enum`` would raise on these circumstances. And we should raise one - from ``pcapkit.utilities.exceptions`` rather builtin exceptions."* The - *shape* is unchanged by that ruling and deliberately so -- a name miss + Both failure paths raise from :mod:`pcapkit.utilities.exceptions` + rather than a builtin, per 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."* The *shape* is unchanged by that ruling and + deliberately so -- a name miss stays :exc:`KeyError`-derived and a value miss :exc:`ValueError`-derived, matching ``E['nosuch']`` and ``E(999)`` on a stdlib :class:`~enum.Enum`, and matching the 119 of this tree's 127 concrete diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index a6caaf72f1..17ef3836e2 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -65,7 +65,7 @@ #: The sum therefore wants a budget, and this is the figure that makes one #: *safe*. A budget on its own is not: a capture cut short by its snapshot length #: pads legitimately and must keep parsing (#431, and the reasoning that declined -#: #571), and it pads far more than it reads, so any running budget tight enough +#: #554), and it pads far more than it reads, so any running budget tight enough #: to matter starts refusing real captures. Worse, it refuses them *sometimes* -- #: measured on this tree with a running budget alone, the same legitimate #: 54-octet frame parsed to one result on 37 of 40 calls and to another on calls @@ -207,7 +207,7 @@ def _zero_pad_budget() -> 'Iterator[list[int]]': #: sign after it (``'>Xs'``), or any other malformed template -- which is what #: makes checking for it a reliable way to tell those cases apart from each #: other *before* :func:`struct.calcsize` is asked to size either one. See -#: #825, #827. +#: #825. _RE_NEGATIVE_LENGTH_TEMPLATE = re.compile(r'^[@=<>!]?-\d+') @@ -542,7 +542,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': # what keeps the answer a function of *this* read rather than of # everything read before it. No shortfall a 16-bit wire length can produce # -- which is every shortfall a snapshot-truncated capture, a truncated - # option area or an over-long ``ihl`` can produce (#431, #571) -- is ever + # option area or an over-long ``ihl`` can produce (#431, #554) -- is ever # refused, on the first frame or the ten-thousandth. # # Nor may those small shortfalls *spend* the budget, which is why they are diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index 4260a1963e..ece4ef9357 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -90,12 +90,12 @@ def _reject_bool(value: 'object', description: str) -> 'None': warning**. On an IPv6-typed field the same conversion happens to raise instead, because the resulting :class:`~ipaddress.IPv4Address`'s version mismatches -- and that - asymmetry is exactly what let this slip past #481's otherwise + asymmetry is exactly what let this slip past #469's otherwise equivalent guard for :meth:`MH._make_opt_mn_id ` (c.f. #491). Every caller checks this *before* dispatching on the value's type, - for the same placement reason #481 gives: a correct check in the + for the same placement reason #469 gives: a correct check in the wrong position does not fire, and that placement mistake has already been made twice in this repository's history. @@ -147,7 +147,7 @@ def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str', length is the only thing on the wire that carries the family -- must convert the argument itself, and that conversion happens **before** the schema, so it launders a :obj:`bool` into an - :class:`~ipaddress.IPv4Address` that #500's guard in + :class:`~ipaddress.IPv4Address` that the guard added for #491 in :meth:`_IPAddressField.pre_process` can then only see as a legitimate address. Seven such call sites took ``True`` / ``False`` without complaint as ``0.0.0.1`` / ``0.0.0.0`` -- or ``::1`` / ``::`` where the @@ -159,7 +159,7 @@ def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str', unrelated defect of its own. Routing every one of them through here rather than giving each its own - :func:`isinstance` check is the whole point: #481 added exactly such a + :func:`isinstance` check is the whole point: #469 added exactly such a check to :meth:`MH._make_opt_mn_id `, and #491 was the same defect surviving at every site that had not been thought of. A @@ -167,14 +167,14 @@ def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str', forgotten at the next one. The :obj:`bool` rejection is the **first** statement here, ahead of any - dispatch on the value's type, for the placement reason #481 gives and + dispatch on the value's type, for the placement reason #469 gives and :func:`_reject_bool` repeats. This raises :exc:`FieldValueError` and not :exc:`~pcapkit.utilities.exceptions.ProtocolError`, which is deliberate even though two sibling guards for the same mistake -- :meth:`MH._make_opt_mn_id - ` from #481 and + ` from #469 and :class:`ESP's SecurityAssociation ` from #491 -- raise the latter. The layer decides: this is a field-level conversion, so it diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index d2fdcde41f..893d3adc32 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -54,7 +54,7 @@ class NumberField(Field[int], Generic[_T]): ProtocolError: If ``bit_length`` is given negative. Left alone, ``(1 << bit_length) - 1`` raises a bare, uncatchable :exc:`ValueError` (``negative shift count``) here, before - :meth:`__call__`'s own negative-``length`` guard (#828/#829) or + :meth:`__call__`'s own negative-``length`` guard (#828) or :attr:`~pcapkit.corekit.fields.field.FieldBase.length`'s (#805) ever see anything -- this one fires at construction time, on the argument itself rather than on a resolved wire length. See @@ -153,7 +153,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': a bare, uncatchable :exc:`ValueError` (``negative shift count``) when ``bit_length`` was not supplied, before :attr:`~pcapkit.corekit.fields.field.FieldBase.length` (see - its own :exc:`ProtocolError` guard, #805/#811/#827) or + its own :exc:`ProtocolError` guard, #805/#825) or :meth:`build_template` ever sees the value: this method sets ``self._bit_length`` from the resolved length eagerly, as a cache, and shifts by it immediately, so the crash happens on diff --git a/pcapkit/corekit/sentinels.py b/pcapkit/corekit/sentinels.py index 09daa7956d..9b046038d6 100644 --- a/pcapkit/corekit/sentinels.py +++ b/pcapkit/corekit/sentinels.py @@ -238,22 +238,24 @@ class NoDefaultType: """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for :meth:`EnumLookup.get `. - A dedicated class rather than a bare :class:`object`, per the owner's ruling - on #859: *"use dedicated class rather than bare object. Follow the house - convention."* A bare :class:`object` compares under ``is`` exactly as - safely as a dedicated class with no ``__eq__`` of its own does -- identity - comparison was never the problem an earlier revision's docstring here - overstated it to be. What a bare :class:`object` actually lacks is a - readable representation: it prints as ```` in a - signature, in :func:`help`, and in a traceback, where ``NoDefaultType()`` + A dedicated class rather than a bare :class:`object`, per a ruling given in + review of the work for #857: *"use dedicated class rather than bare object. + Follow the house convention."* A bare :class:`object` compares under + ``is`` exactly as safely as a dedicated class with no ``__eq__`` of its + own does -- identity comparison was never the problem an earlier + revision's docstring here overstated it to be. What a bare + :class:`object` actually lacks is a readable representation: it prints + as ```` in a signature, in :func:`help`, and in + a traceback, where ``NoDefaultType()`` -- via :meth:`__repr__` below -- prints as ````. Named ``NoDefaultType`` for the *class* because that half of the house convention is settled: both :class:`NullType` and :class:`NoValueType` use ``Type``. At the time, the *instance*'s own name was not - similarly settled -- the owner's follow-up on #859 was explicit that - ``NULL`` (``SCREAMING_CASE``) and ``NoValue`` (``CapWords``) disagreed, and - "mainly depends on how we need it." The need here was continuity: + similarly settled -- a follow-up given in review of the work for #857 + was explicit that ``NULL`` (``SCREAMING_CASE``) and ``NoValue`` + (``CapWords``) disagreed, and "mainly depends on how we need it." The + need here was continuity: ``NO_DEFAULT`` was already the name on ``main`` -- referenced in :meth:`EnumLookup.get `'s signature, its docstring, and both comparison sites -- and that change was to *what @@ -358,7 +360,7 @@ class NoDefaultType: compares by value rather than identity, and reload staleness is a *tracked* defect class here for other constructs -- see :meth:`pcapkit.protocols.protocol.ProtocolBase._lookup_next_layer`'s own - docstring note citing GitHub issues #425, #428 and #560, and + docstring note citing GitHub issues #425 and #555, and :mod:`tests.protocols.test_dispatch_default_resolution_unit`'s own ``test_no_stale_class_survives_a_module_reload``, which reloads a module deliberately to pin the fix for exactly that class of bug elsewhere. A @@ -454,13 +456,13 @@ class AbsentType: normalised every sentinel *object* to SCREAMING_SNAKE and dropped it, so this pair now reads as CamelCase/SCREAMING_SNAKE like their two siblings and privacy is no longer signalled by the name at all. The owner'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 this class and :data:`ABSENT` stay exactly as private - as they were: nothing outside :mod:`pcapkit.protocols.protocol` reads - :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 + on GitHub issue #719, 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 this class and :data:`ABSENT` stay + exactly as private as they were: nothing outside + :mod:`pcapkit.protocols.protocol` reads :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/sentinel-convention.rst`, are what now records that fact in place of the leading underscore. diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index 862582b04b..1106dc4c47 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -887,11 +887,11 @@ def register_apptype(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor # up front, before any registry work, so every downstream use -- the # ``code.proto`` default and the registries lookup below -- sees members # only. Resolution is by the member's own ``name``, case-insensitively -- - # maintainer ruling on #815 -- via ``__members__`` directly rather than - # ``TransportProtocol[name]``: this function's contract is - # :exc:`~pcapkit.utilities.exceptions.RegistryError` for anything - # unrecognised, composite-spelled or not, and ``__getitem__`` raises a - # bare :exc:`KeyError` on a miss instead of that -- both + # a ruling given in review of the work for #806 -- via ``__members__`` + # directly rather than ``TransportProtocol[name]``: this function's + # contract is :exc:`~pcapkit.utilities.exceptions.RegistryError` for + # anything unrecognised, composite-spelled or not, and ``__getitem__`` + # raises a bare :exc:`KeyError` on a miss instead of that -- both # ``TransportProtocol['tcp|udp']`` and ``TransportProtocol['bogus']`` do, # now that GitHub issue #808 dropped the ``IntFlag`` base that used to # make the first of those two silently compose into the value ``3`` diff --git a/pcapkit/protocols/application/http.py b/pcapkit/protocols/application/http.py index 22b0e94783..a7d337838a 100644 --- a/pcapkit/protocols/application/http.py +++ b/pcapkit/protocols/application/http.py @@ -226,7 +226,7 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': preface came back ``version='2'`` only because ``httpv2.HTTP`` read its leading ``b'PRI'`` as a 24-bit declared frame length of 5,265,993, and ``b'foo bar baz\\r\\nX: y\\r\\n\\r\\n'`` -- not HTTP at all -- came back - ``version='2'`` the same way. #799/#802 closed the second of those by + ``version='2'`` the same way. #799 closed the second of those by requiring a frame's declared length to be backed by its buffer, but that left the preface *unidentifiable*: a real HTTP/2 connection opening is refused by both arms and reported as not-HTTP. @@ -313,7 +313,7 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # version``, so a preface followed by a frame that used to trip the # #805 residual (an inner field shortfall, e.g. a 16-octet # ``GOAWAY``) had to reach the caller as something it could catch, - # not as a bare stdlib error. #811 has since closed that residual at + # not as a bare stdlib error. That residual has since been closed at # ``FieldBase.length``, so the same ``GOAWAY`` now raises # ``ProtocolError`` on its own and is caught by the ``except # ProtocolError: raise`` above, never reaching this clause -- but @@ -377,8 +377,8 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # closed that particular route -- the same call now raises # ``ProtocolError: unknown HTTP version``, the documented answer. # Suppressing :exc:`struct.error` on the last arm stays regardless -- - # #811 kept it deliberately, as defence in depth, rather than retiring - # it now that the case it was added for is closed. Whether anything + # kept deliberately, as defence in depth, rather than retired now + # that the case it was added for is closed. Whether anything # can still reach it, and whether it should therefore go, is #825's # open question, not settled here. # @@ -396,7 +396,7 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # field further in, past this guard's reach. Measured: a 16-octet # ``GOAWAY`` (``b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7``) # used to raise a bare :exc:`struct.error` through ``httpv2.HTTP`` - # directly. #811 closed that class at its actual root -- + # directly. That class was closed at its actual root -- # :attr:`FieldBase.length # ` now catches # :func:`struct.calcsize`'s failure on a negative-length template and diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index bb1033c4e2..a0ff409f3e 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -259,10 +259,10 @@ class HIP(IPv6_Ext[Data_HIP, Schema_HIP], Internet[Data_HIP, Schema_HIP], schema=Schema_HIP, data=Data_HIP): """This class implements Host Identity Protocol. - Double-inherited, per the maintainer's convention on GitHub pull request - #924: a header that is *only* usable as an extension header inherits - :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` alone, while one - that is also usable as a standalone protocol names + Double-inherited, per the maintainer's convention given in review of the + work for #917: a header that is *only* usable as an extension header + inherits :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` alone, + while one that is also usable as a standalone protocol names :class:`~pcapkit.protocols.internet.internet.Internet` as well. HIP is both, on two independent grounds: diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index 973075e01e..6f9b5a2ea2 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -478,12 +478,12 @@ def _hopopt_option_length(schema_len: 'int') -> 'int': excludes the Option Type and Opt Data Len fields themselves, so the whole option, which is what every ``_read_opt_*`` below reports back as the parsed option's own ``.length``, is two octets more. This is - the exact ``+2``/``-2`` mismatch #398 fixed independently in six + the exact ``+2``/``-2`` mismatch independently fixed in six places (see ``Data_PadOption.length`` vs. ``Schema_PadOption.length`` below, at the surviving explanation of that fix); collecting the read-side half of it into one helper is so a future fix to this arithmetic only has to happen once. Do NOT drop the ``+ 2``: that is - precisely the mismatch #398 fixed. + precisely this mismatch. Section 4.2 is the citation because it is what defines the TLV option format, and it is where the sentence quoted above actually appears. diff --git a/pcapkit/protocols/internet/ipv6_opts.py b/pcapkit/protocols/internet/ipv6_opts.py index b8a0b65a1c..17e8e291bb 100644 --- a/pcapkit/protocols/internet/ipv6_opts.py +++ b/pcapkit/protocols/internet/ipv6_opts.py @@ -470,12 +470,12 @@ def _ipv6_opts_option_length(schema_len: 'int') -> 'int': excludes the Option Type and Opt Data Len fields themselves, so the whole option, which is what every ``_read_opt_*`` below reports back as the parsed option's own ``.length``, is two octets more. This is - the exact ``+2``/``-2`` mismatch #398 fixed independently in six + the exact ``+2``/``-2`` mismatch independently fixed in six places (see ``Data_PadOption.length`` vs. ``Schema_PadOption.length`` below, at the surviving explanation of that fix); collecting the read-side half of it into one helper is so a future fix to this arithmetic only has to happen once. Do NOT drop the ``+ 2``: that is - precisely the mismatch #398 fixed. + precisely this mismatch. Section 4.2 is the citation because it is what defines the TLV option format, and it is where the sentence quoted above actually appears. diff --git a/pcapkit/protocols/internet/ipv6_route.py b/pcapkit/protocols/internet/ipv6_route.py index 924165a253..fb41e7b8dc 100644 --- a/pcapkit/protocols/internet/ipv6_route.py +++ b/pcapkit/protocols/internet/ipv6_route.py @@ -611,7 +611,7 @@ def _read_data_type_rpl(self, schema: 'Schema_RPL', *, header: 'Schema_IPv6_Rout # ``header.length`` is ``Hdr Ext Len``, in the 8-octet units # :rfc:`6554#section-3` specifies, not octets -- and it additionally # assumed 16-octet addresses, which an SRH only carries when ``CmprI`` - # and ``CmprE`` are both 0. #489 called it out but deliberately left + # and ``CmprE`` are both 0. #487 called it out but deliberately left # it alone, because ``RPL.post_process`` raised on every pack back # then so there was no round trip to validate a replacement against. # #556 removed that blocker and #564 the mis-sized fixed area behind diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index 729400e1f4..23651ebfbc 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -581,11 +581,11 @@ 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. The owner's final ruling on #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 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 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 +698,11 @@ 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. The owner's final ruling on #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 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 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 @@ -838,9 +838,10 @@ class LocalizedRoutingStatus(EnumLookup, IntEnum): :class:`IPv6AddressPrefixCode` either: it had zero callers repo-wide -- 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 the owner's ruling - there, verbatim: *"I prefer (2) directly"* -- ``(2)`` being deletion - of those two overrides rather than widening them to match the base. + #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. 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 @@ -917,9 +918,10 @@ class LMAAddressCode(EnumLookup, IntEnum): :class:`IPv6AddressPrefixCode` either: it had zero callers repo-wide -- 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 the owner's ruling - there, verbatim: *"I prefer (2) directly"* -- ``(2)`` being deletion - of those two overrides rather than widening them to match the base. + #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. 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 @@ -2903,13 +2905,13 @@ def _mh_option_length(schema_length: 'int') -> 'int': Length fields"* -- so the whole option, which is what every ``_read_opt_*`` below reports back as the parsed option's own ``.length``, is two octets more. This is the exact ``+2``/``-2`` - mismatch #398 fixed independently in six places (see the ``Note:`` + mismatch independently fixed in six places (see the ``Note:`` on :meth:`_read_opt_pad` below, which explains why a ``Pad1`` option -- the one option with no ``Option Length`` field at all -- is this helper's sole exception); collecting the read-side half of it into one helper is so a future fix to this arithmetic only has - to happen once. Do NOT drop the ``+ 2``: that is precisely the - mismatch #398 fixed. + to happen once. Do NOT drop the ``+ 2``: that is precisely this + mismatch. The ``+ 2`` is specific to an :rfc:`6275#section-6.2` mobility option, whose Option Type and Option Length are one octet each. It @@ -7972,7 +7974,7 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption (RFC 4283's ``user@realm`` form) rather than a numeric identifier, so there is no non-arbitrary int-to-text mapping the way there is int-to-address or int-to-octets, and an - :obj:`int` is rejected there (c.f. #467, #468). + :obj:`int` is rejected there (c.f. #467). **kwargs: Arbitrary keyword arguments. Returns: @@ -7994,7 +7996,7 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption type its subtype's field cannot hold at all: anything but :obj:`str` for ``NAI``, anything but :obj:`bytes`/ :obj:`bytearray`/:obj:`int` for the other six -- an :obj:`int` - is converted rather than rejected there, per #468 -- or anything + is converted rather than rejected there, per #467 -- or anything :class:`ipaddress.IPv6Address` itself does not accept for ``IPv6_Address`` (c.f. #469). @@ -8015,14 +8017,14 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # exactly the class of leak this handler exists to stop, and which a # guard living inside the ``elif isinstance(identifier, int)`` branch # could not catch, since the ``IPv6_Address`` dispatch never reaches - # it (c.f. #467, #468). + # it (c.f. #467). try: # ``Enum_MNIDSubtype(subtype_val)`` round-trips a plain int back # into a named member for the message below -- but its own # ``_missing_`` only auto-extends 9-15 and 16-255, so 0, # negatives and anything above 255 make the constructor itself # raise a bare ``ValueError``, which would defeat the point of - # this guard (c.f. #468 review). Caught here and the raw value + # this guard (c.f. #467). Caught here and the raw value # used instead rather than let it propagate. subtype_repr = repr(Enum_MNIDSubtype(subtype_val)) except ValueError: @@ -8063,7 +8065,7 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # unguarded, :class:`ipaddress.IPv6Address` raises # ``AddressValueError``, itself a bare :exc:`ValueError`, so this # handler would otherwise ship with its lower bound guarded and its - # upper bound leaking (c.f. #467, #468). Checked explicitly rather + # upper bound leaking (c.f. #467). Checked explicitly rather # than by wrapping the construction below, because that would also # swallow the wrong-*type* ``AddressValueError`` -- a ``str`` or # ``None`` reaching here -- which is #469's subject, not this one's. @@ -8080,7 +8082,7 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # bytes, int or str). The ``try`` wraps only this call, not # the whole branch, so it cannot swallow the ProtocolError # raised above for an out-of-range int, which is also a - # ValueError subclass (c.f. #467, #468, #469). + # ValueError subclass (c.f. #467, #469). try: identifier = ipaddress.IPv6Address(identifier) except ValueError as error: @@ -8114,9 +8116,9 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption # width all along -- the pre-#467 defect was never the sizing, it # was that ``identifier`` itself stayed an ``int`` afterwards and # was handed to ``BytesField`` unconverted, which ``struct.pack()`` - # cannot do anything with. #468 initially rejected outright instead + # cannot do anything with. #467 initially rejected outright instead # of noticing that; converting is what this revision does (c.f. - # #467, #468). ``bit_length()`` is 0 for 0 itself, which would + # #467). ``bit_length()`` is 0 for 0 itself, which would # otherwise declare a zero-octet identifier -- collapsing "the # identifier's value is 0" into "there is no identifier" -- so the # width is floored at one octet, matching what any reasonable diff --git a/pcapkit/protocols/link/ospf.py b/pcapkit/protocols/link/ospf.py index 9438cad27b..695957a31f 100644 --- a/pcapkit/protocols/link/ospf.py +++ b/pcapkit/protocols/link/ospf.py @@ -336,7 +336,7 @@ def _make_id_numbers(self, id: 'IPv4Address | str | bytes | bytearray') -> 'byte straight from its own arguments), so the only caller is a unit test. It is routed through :func:`parse_ip_address` anyway, so that it does not resurface the defect the moment a caller - reaches it -- the same kind of omission is how #481's single-site + reaches it -- the same kind of omission is how #469's single-site fix survived to become #491 and then #508 (c.f. #540). The description below uses :attr:`self.__class__.__name__ diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index e28de84ff4..a391b386cc 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -278,7 +278,7 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, # other way round, which is what this reader used to do (see #618). # # The two only differ for a frame the snapshot length cut short, so - # until ``big_endian.pcap`` arrived with #614 no fixture here could + # until ``big_endian.pcap`` arrived with #605 no fixture here could # tell the two assignments apart. # # Worth knowing *why* this reader moved rather than the other one, diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 9abcc97ee5..1f4033c540 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -293,14 +293,15 @@ class PacketReception(EnumLookup, enum.IntEnum): # :class:`pcapkit.const.pcapng.tls_key_label.TLSKeyLabel`, generated the same # way as its :mod:`pcapkit.const.pcapng` siblings -- imported above as # ``Enum_TLSKeyLabel``, matching the ``Enum_*`` alias every one of its seven -# :mod:`pcapkit.const.pcapng` siblings already carries in this file (the -# owner's review on #890: this import was the only one of the eight lacking -# it, because the class used to be *defined* here rather than imported, so -# nothing applied the convention until this move made it an import). The -# assignment below re-exports the same object under the module's own, -# unaliased name, so ``from pcapkit.protocols.misc.pcapng import -# TLSKeyLabel`` keeps working and still resolves to the identical class -- -# not a copy -- that every internal ``Enum_TLSKeyLabel`` reference below uses. +# :mod:`pcapkit.const.pcapng` siblings already carries in this file (a +# review of the work for #886: this import was the only one of the +# eight lacking it, because the class used to be *defined* here rather than +# imported, so nothing applied the convention until this move made it an +# import). The assignment below re-exports the same object under the +# module's own, unaliased name, so ``from pcapkit.protocols.misc.pcapng +# import TLSKeyLabel`` keeps working and still resolves to the identical +# class -- not a copy -- that every internal ``Enum_TLSKeyLabel`` reference +# below uses. # :class:`WireGuardKeyLabel` below stays hand-written -- it is verified closed # (draft-ietf-opsawg-pcapng-06 section 4.7's "is one of" four names) and not a # registry, so filing it under :mod:`pcapkit.const` would misrepresent it as diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 2a5c5bacdc..7ae245ccea 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -1741,8 +1741,8 @@ def _lookup_next_layer(registry: 'DefaultDict[int, ModuleDescriptor[ProtocolBase :func:`importlib.import_module` -- see #574. Memoising the resolved class here instead, whether under ``proto``, in ``registry``'s default factory, or in a cache beside the registry, would retain a - class that :func:`importlib.reload` then makes stale; #425 and #428 - at this layer and #560 at the schema layer are all that same defect. + class that :func:`importlib.reload` then makes stale; #425 at + this layer and #555 at the schema layer are all that same defect. """ protocol = ProtocolBase._lookup_registry(registry, proto) diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 1cbf147893..406a0f72d8 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -572,7 +572,7 @@ class R1CounterParameter(Parameter, code=[Enum_Parameter.R1_Counter, layout to code 128 (``R1_Counter``) and code 129 (``R1_COUNTER``) -- one parameter under two numbers, the difference being HIP's own C-bit rather than an unrelated code -- and the field list below is that layout - exactly, since #696 widened :attr:`counter` to eight octets. + exactly, since #672 widened :attr:`counter` to eight octets. :attr:`~pcapkit.protocols.internet.hip.HIP.__parameter__` already carries two hand-written entries -- not a name-normalisation rule; ``R1_Counter`` and ``R1_COUNTER`` differ only in case, and each needed its own line -- @@ -681,9 +681,10 @@ class LocatorSetParameter(Parameter, code=Enum_Parameter.LOCATOR_SET): #: parameter's packet context while they pack, and their own ``len`` #: overwrites it before ``padding`` is reached. #: - #: This is the site #651 deliberately left alone and #664 documented as an - #: exclusion, because two defects in this parameter cancelled at the shape - #: its tests sampled and correcting either alone made the wire output worse. + #: This is the site #651 deliberately left alone, and its fix + #: documented as an exclusion, because two defects in this parameter + #: cancelled at the shape its tests sampled and correcting either alone + #: made the wire output worse. #: The shadowed ``len`` is 4 for any IPv6 locator, so the old expression #: appended exactly four octets whatever the locator count; and the wrong #: ``Length`` unit above made the declared ``Length`` ``4n`` where the diff --git a/pcapkit/protocols/schema/internet/ipv4.py b/pcapkit/protocols/schema/internet/ipv4.py index ca9b9b1767..393db80bb7 100644 --- a/pcapkit/protocols/schema/internet/ipv4.py +++ b/pcapkit/protocols/schema/internet/ipv4.py @@ -368,8 +368,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': of, so nothing downstream can question it. Measured before this fix: ``ts_data=[True, 5]`` packed as ``0000000100000005`` and reported ``IPv4Address('0.0.0.1')`` with no exception and no - warning. This was the fifth site of that defect -- #481, #500, - #539 and #540 are the first four -- and the reason it is the fifth + warning. This was the fifth site of that defect -- #469, #491, + #508 and #540 are the first four -- and the reason it is the fifth is that each of those fixed the sites it could see. See #552. """ @@ -434,7 +434,7 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': # still raises a plain :exc:`ValueError` for a tail that is not a # whole number of 8-octet pairs, which no ``except BaseError`` can # catch, and because leaving one of this method's three conversions - # unguarded is exactly how #552 came to be the fifth site of #481. + # unguarded is exactly how #552 came to be the fifth site of #469. pad = self.remainder for index in range(0, len(pad), 8): buf_ip = pad[index:index + 4] diff --git a/pcapkit/protocols/schema/internet/ipv6_route.py b/pcapkit/protocols/schema/internet/ipv6_route.py index f480cebb1c..83ac62b7fa 100644 --- a/pcapkit/protocols/schema/internet/ipv6_route.py +++ b/pcapkit/protocols/schema/internet/ipv6_route.py @@ -81,10 +81,10 @@ def ipv6_route_header_length(hdr_ext_len: 'int') -> 'int': :mod:`pcapkit.protocols.internet.ipv6_route` reports back as the parsed route data's own ``.length``, which :meth:`~pcapkit.protocols.internet. ipv6_route.IPv6_Route.read` then subtracts from the outer packet length - to find the next layer's length. #489 unified the write side + to find the next layer's length. #487 unified the write side (:meth:`~pcapkit.protocols.internet.ipv6_route.IPv6_Route._make_hdr_ext_len`) into one helper; this is the matching read-side helper for the total - header length, finishing that half of #487/#489. + header length, finishing that half of #487. Args: hdr_ext_len: raw ``Hdr Ext Len`` field value, as read off the wire. diff --git a/pcapkit/protocols/schema/internet/mh.py b/pcapkit/protocols/schema/internet/mh.py index 7b475ecfc3..995b8c534e 100644 --- a/pcapkit/protocols/schema/internet/mh.py +++ b/pcapkit/protocols/schema/internet/mh.py @@ -741,7 +741,7 @@ class MNIDOption(Option, code=Enum_Option.MN_ID_OPTION_TYPE): # :class:`~pcapkit.corekit.fields.strings.StringField` or # :class:`~pcapkit.corekit.fields.strings.BytesField`, and handing either # a raw ``int`` is precisely the #467 defect -- ``struct.pack()`` cannot - # consume it (c.f. #467, #468). + # consume it (c.f. #467). def __init__(self, type: 'Enum_Option', length: 'int', subtype: 'Enum_MNIDSubtype', identifier: 'bytes | str | IPv6Address') -> 'None': ... diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 4cbe91b294..a40d1d1937 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -316,12 +316,12 @@ def bounded_option(length: 'Callable[[dict[str, Any]], int]') -> 'Callable[[dict with one option declaring 65,535 against none present, produced 131,070,000 octets of zero padding, an amplification of 1,637x linear in the block count. See `#594 `__, - and `#593 `__ for the + and `#573 `__ for the 32-bit band the field layer's own budget already covers. The bound has to come from this layer because the field layer cannot see it. What distinguishes the crafted case from the legitimate one is not the - shortfall's size -- both are inside a 16-bit length, which is why #571's + shortfall's size -- both are inside a 16-bit length, which is why #554's ``len(buffer) < length`` rejection was declined -- but whether the option is inconsistent with the framing the block itself declares. Block Total Length is authoritative and cross-checked against its own trailing copy, so @@ -804,7 +804,7 @@ def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = every other registry in the package does -- the lookup that follows cannot tell a deliberate replacement from an accidental one, so an unreported overwrite is a parser silently swapped out for another. See - `#681 `__ for the + `#675 `__ for the guard ``register_protocol`` added first, which this one now matches. The guard is identity-based: it fires only when the incumbent differs @@ -1865,8 +1865,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': names them, and a blank line -- found by *reading*, not by splitting -- is what starts the next entry. The one-octet terminator that must follow a binary field's value, and the warning - when it is missing, are unchanged from `#722 - `__; walking the + when it is missing, are unchanged from `#704 + `__; walking the buffer whole rather than pre-slicing it also retires that fix's newline restoration, which existed only to undo what the slicing itself had taken away. @@ -2016,10 +2016,10 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': # the reader's position is well defined either way -- # exactly length + 1 octets past where the field name # started -- so a bad octet here ends only this - # entry's field collection, matching #722: it does not + # entry's field collection, matching #704: it does not # abort the walk, which keeps looking for the next - # entry's separator from here. See #728's review for - # why an outer abort was considered and rejected as + # entry's separator from here. See #723 for why an + # outer abort was considered and rejected as # the default. terminator = entry_data.read(1) if terminator != b'\n': diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index ddb595bc69..e2e79640bb 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -1065,7 +1065,7 @@ class _EnumRegistry(collections.defaultdict): This is the schema-layer instance of the defect :meth:`ProtocolBase.\ _lookup_registry ` fixed for the protocol-layer ``__proto__`` family in GitHub issues #421 and - #425/#428; see GitHub issue #555. The fallback itself is deliberate -- it + #425; see GitHub issue #555. The fallback itself is deliberate -- it is how an unknown option, chunk or block falls back to its ``Unknown*``/``Unassigned*`` schema -- so this subclass keeps returning it, it just stops recording it. @@ -1305,7 +1305,7 @@ def register(cls, code: '_ET', schema: 'Type[Self]') -> 'None': have made the next legitimate registration for that code warn about an entry no caller ever asked for -- the defect fixed for this layer in #555, and for the parser-layer ``__proto__`` family in #421 and - #425/#428. That fix is what makes this guard safe to add. + #425. That fix is what makes this guard safe to add. :class:`pcapkit.protocols.schema.misc.pcapng.Option` overrides this method with a namespaced registry of its own and does not delegate diff --git a/pcapkit/protocols/schema/transport/tcp.py b/pcapkit/protocols/schema/transport/tcp.py index 07fbef5564..b3272436d9 100644 --- a/pcapkit/protocols/schema/transport/tcp.py +++ b/pcapkit/protocols/schema/transport/tcp.py @@ -276,7 +276,7 @@ def mptcp_dss_ack_selector(pkt: 'dict[str, Any]') -> 'Field': integer``. Measured on the 8-octet form, which the old lambda did reach: ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised exactly that. - That half is now history: **#598 fixed it**, in + That half is now history: **#591 fixed it**, in :mod:`pcapkit.corekit.fields.numbers` where this note used to say the fix belonged, by recomputing ``_need_process`` from the width actually in force instead of once from the placeholder. A callable-length @@ -334,8 +334,8 @@ def mptcp_dss_dsn_selector(pkt: 'dict[str, Any]') -> 'Field': identical defect: ``NumberField(length=lambda pkt: 8 if pkt['flags']['m'] else 0, ...)``. See that function's note for why the ``0`` was wrong, why a corrected lambda would not have packed either *at the time*, and why the - ``SwitchField`` form is kept now that #598 has made a callable length work. - C.f. #576, #598. + ``SwitchField`` form is kept now that #591 has made a callable length work. + C.f. #576, #591. """ if not pkt['flags']['M']: @@ -409,12 +409,12 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ A port outside this field's own width is rejected *before* any of that, rather than being let through to :meth:`_missing_` and caught alongside a genuine miss. Both are a bare :exc:`ValueError` - with nothing to tell them apart by type, and GitHub issue #764 + with nothing to tell them apart by type, and GitHub issue #758 gave the out-of-range case a deliberate, ``breaking``-tagged rejection specifically so it would stop being minted over -- a catch keyed on exception type alone cannot see the difference between that and :mod:`aenum`'s own "no member has this value", - so it would absorb both and quietly revert #764 for these four + so it would absorb both and quietly revert #758 for these four fields. Checking the width first needs no exception-based distinction at all: it asks the same question :meth:`_missing_` would eventually ask, and asks it in a way that never manufactures @@ -432,7 +432,7 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ value = super(EnumField, self).post_process(value, packet) proto = Enum_TransportProtocol.tcp if not (isinstance(value, int) and 0 <= value < (1 << (8 * self.length))): - # NOTE: lets AppType.get() -- unmodified -- raise #764's rejection + # NOTE: lets AppType.get() -- unmodified -- raise #758's rejection # for a port this field's own width cannot represent, rather than # risking it being absorbed below as a foreign miss. return self._namespace.get(value, proto=proto) @@ -444,7 +444,7 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ # NOTE: value is already known to be in-width here, so this # ValueError is aenum's own "no member has this value" for an # in-range but unassigned port -- a foreign miss, absorbed -- - # never #764's out-of-range rejection, which never reaches + # never #758's out-of-range rejection, which never reaches # this branch. A pcapkit.utilities.exceptions error is still a # deliberate registry decision and propagates unchanged. if isinstance(error, BaseError): diff --git a/pcapkit/utilities/decorators.py b/pcapkit/utilities/decorators.py index 1b59496dec..19999cb785 100644 --- a/pcapkit/utilities/decorators.py +++ b/pcapkit/utilities/decorators.py @@ -172,7 +172,7 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder': # whether or not it is registered. So both *unknown* paths preserve # the code that arrived; what differs is only that SCTP's is an # enumeration the protochain can name, while TCP's is an ``int`` and - # renders as ``Raw``. That was #418, closed by #426; this line is + # renders as ``Raw``. That was #418; this line is # about the *failure* path, which it makes uniform. next_ = protocol(file_, length, error=str(exc), alias=proto) return cast('R_beholder', next_) diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 78f796b194..be86a13b33 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -716,7 +716,7 @@ 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. The owner's ruling on GitHub issue #923, + :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."* diff --git a/pcapkit/vendor/__main__.py b/pcapkit/vendor/__main__.py index 814b985ff3..92eb770e4f 100644 --- a/pcapkit/vendor/__main__.py +++ b/pcapkit/vendor/__main__.py @@ -51,11 +51,12 @@ 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. - The owner's ruling on GitHub issue #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, 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. It is a *wider* guarantee than protecting the single ``open``/``print`` pair :meth:`~pcapkit.vendor.default.Vendor.__init__` diff --git a/pcapkit/vendor/ftp/command.py b/pcapkit/vendor/ftp/command.py index e97c9d7abc..5ec3ff1091 100644 --- a/pcapkit/vendor/ftp/command.py +++ b/pcapkit/vendor/ftp/command.py @@ -75,7 +75,7 @@ class FEATCode(EnumRegistry, StrEnum): :meth:`~pcapkit.vendor.ftp.command.Command.process`) -- rather than minting the per-command ones at import time as an incidental side effect of building :class:`Command`'s own rows. GitHub issue #860: - that import-time mutation was the same defect shape #861 removed + that import-time mutation was the same defect shape #775 removed from :class:`~pcapkit.const.pcapng.filter_type.FilterType`, just not previously noticed here. ``_missing_`` still unmints for a keyword that turns up on the wire but names none of these -- the diff --git a/pcapkit/vendor/ipx/socket.py b/pcapkit/vendor/ipx/socket.py index 8809f93a14..b470148f2a 100644 --- a/pcapkit/vendor/ipx/socket.py +++ b/pcapkit/vendor/ipx/socket.py @@ -75,7 +75,7 @@ # NOTE: 0x0000 was never listed as a well-known socket in the scraped # registry table, but it is the IPX protocol's own default for the # ``dst``/``src`` socket field (an ordinary "unspecified socket"), so it - # must be present regardless of what the registry says; see #492, #503. + # must be present regardless of what the registry says; see #492. # # That is a statement about the *scraped table*, not about the number being # unsourced. XSIS 028112 Appendix D (cited under RANGES below) reserves it @@ -252,7 +252,7 @@ #: Range names from :data:`RANGES` above that name an allocation *policy* for #: the pool (who may claim a code, or that nobody has) rather than a specific -#: assigned protocol. The owner's original ruling on #775/#847 held +#: assigned protocol. The owner's original ruling on #775/#841 held #: ``Registered by Xerox`` out of this set as a real ownership fact rather #: than a placeholder. #775's final round converts it too, so this set no #: longer decides *whether* a range registers -- only *how its name is diff --git a/pcapkit/vendor/pcapng/tls_key_label.py b/pcapkit/vendor/pcapng/tls_key_label.py index 3f550dac2a..4e4655b9ca 100644 --- a/pcapkit/vendor/pcapng/tls_key_label.py +++ b/pcapkit/vendor/pcapng/tls_key_label.py @@ -24,7 +24,7 @@ __all__ = ['TLSKeyLabel'] #: Hand-carried comment for the ``RSA`` member, copied verbatim from the text -#: GitHub issue #883 gave it in +#: GitHub issue #882 gave it in #: :class:`pcapkit.protocols.misc.pcapng.TLSKeyLabel` -- ``RSA`` has no row in #: :attr:`TLSKeyLabel.LINK`'s registry to generate a comment from (``grep -c #: RSA`` on the fetched CSV is 0), so it is carried across rather than @@ -134,7 +134,7 @@ def process(self, data: 'list[str]') -> 'tuple[list[str], list[str]]': pref = f"{renm} = '{label}'" # NOTE: mirrors the ``# nosec B105`` bandit suppression every # such member already carries in - # :class:`pcapkit.protocols.misc.pcapng.TLSKeyLabel` as of #883 -- + # :class:`pcapkit.protocols.misc.pcapng.TLSKeyLabel` as of #882 -- # bandit's B105 (hardcoded password string) flags a string # constant assigned to a name containing ``SECRET``, which eight # of these ten labels' names do. diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index d7a5b24bf9..9b13a4ce52 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -136,12 +136,12 @@ class TransportProtocol(EnumLookup, IntEnum): # ``undefined`` is declared explicitly as ``0`` and every other member # is ``auto()``, which continues from the preceding explicit value # rather than needing its own ``_start_ = 0`` to begin there; the - # owner's own ruling on this issue (#860) is explicit that + # a ruling given in review of the work for #860 is explicit that # ``undefined`` stays a direct ``0`` for that reason: "undefined # direct uses 0. then other real transport use auto. so we don't have # to define a _start_ and the undefined declaration is explicit." - # GitHub issue #808 already dropped the ``IntFlag`` base once - # nothing built a composite, and GitHub PR #836's ruling later + # GitHub issue #808 already dropped the ``IntFlag`` base once nothing + # built a composite, and a ruling given in review of that work later # retired ``|``-composite decoding entirely: "since it's no longer a # Flag, `|` joined values are no longer parsed and accepted, we will # treat it as a whole, instead of splitting." With no decoding left to @@ -154,7 +154,7 @@ class TransportProtocol(EnumLookup, IntEnum): # bare -- into anything narrower than the whole value it already is, on # either numbering; a plain ``dict.get`` lookup cannot tell a composed # ``int`` from any other one that happens to equal it. Treating every - # int as a whole is exactly what #836's ruling above asks for, and it + # int as a whole is exactly what that ruling above asks for, and it # is also why this renumbering changes what specific ints mean, not # only what composed ones do: # ``tcp | udp`` (``3``) used to name no registry and now resolves as @@ -201,22 +201,23 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco on these circumstances."* A stdlib ``E['nosuch']`` raises :exc:`KeyError`, and #923's census of the 127 concrete :class:`~pcapkit.corekit.enum.EnumLookup` subclasses -- taken before - #921 re-parented this class, so this class is not among them -- found - 119 already answering a name miss that way against 5 answering with - :exc:`ValueError`. Those 5 are :class:`AppType` and its four transport - registries, and they land there only because their own ``get()`` takes - an :class:`int` port and never accepts a name at all, rather than from - any name-miss policy. So there was no policy here to preserve, and a - name miss now reaches the caller as + the phase-2 re-parenting moved this class, so it is not among them -- + found 119 already answering a name miss that way against 5 answering + with :exc:`ValueError`. Those 5 are :class:`AppType` and its four + transport registries, and they land there only because their own + ``get()`` takes an :class:`int` port and never accepts a name at all, + rather than from any name-miss policy. So there was no policy here + to preserve, and a name miss now reaches the caller as :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from the base. - Maintainer ruling on GitHub PR #836 -- "Do not allow extension of - TransportProtocol at all" -- is untouched by that: the refusal is still - a refusal and still mints nothing, only its exception class moved. + Maintainer ruling given in review of the work for #808 -- "Do not + allow extension of TransportProtocol at all" -- is untouched by + that: the refusal is still a refusal and still mints nothing, only + its exception class moved. The base is a :class:`classmethod` (:meth:`~pcapkit.corekit.enum.EnumLookup.get`), so this override moves from :class:`staticmethod` to :class:`classmethod` to - delegate at all -- the same move GitHub issue #908 and #915 made for + delegate at all -- the same move GitHub issue #908 made for :meth:`~pcapkit.const.http.method.Method.get`. Grepped every call site in this tree for GitHub issue #877: all call this method by name, none take it as a bare callable or introspect ``__func__``, @@ -256,19 +257,20 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco """ if isinstance(key, str): return super().get(key.lower(), default) - # NOTE: maintainer ruling on this PR (#836): "Do not allow extension - # of TransportProtocol at all." A name that is not a declared member - # used to mint a brand-new one here, at ``max_val + 1`` (before that, - # ``max_val * 2``) -- an unbounded, ever-growing set of transport - # protocols nothing ever asked for. There is nothing left to walk - # now: it is simply refused, exactly like any other unrecognised - # name -- including one spelling a composite, e.g. ``'tcp|udp'``. + # NOTE: maintainer ruling given in review of the work for #808: "Do + # not allow extension of TransportProtocol at all." A name that is + # not a declared member used to mint a brand-new one here, at + # ``max_val + 1`` (before that, ``max_val * 2``) -- an unbounded, + # ever-growing set of transport protocols nothing ever asked for. + # There is nothing left to walk now: it is simply refused, exactly + # like any other unrecognised name -- including one spelling a + # composite, e.g. ``'tcp|udp'``. # ``'|'`` used to be intercepted here on its own, so a composite in # disguise never got minted into a member whose own name lied about - # being a single transport; the owner's further ruling on this PR - # retired that special case along with the rest of the composite - # handling once TransportProtocol stopped being a Flag at all: - # "since it's no longer a Flag, `|` joined values are no longer + # being a single transport; a further ruling given in review of the + # work for #808 retired that special case along with the rest of the + # composite handling once TransportProtocol stopped being a Flag at + # all: "since it's no longer a Flag, `|` joined values are no longer # parsed and accepted, we will treat it as a whole, instead of # splitting." A ``'|'``-joined name is therefore not special any # more -- it is simply not the name of a declared member, and gets @@ -479,10 +481,10 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ the same as any caller passing a literal port-transport bitmask -- falls through to ``int.__or__`` and returns a bare :class:`int` rather than a member. Never split back into the - transports its bits would each name -- owner ruling on this PR - (#836) -- so it is looked up as the one whole value it - already is, exactly like any other bare int: refused when - that whole value names no registry, resolved when it + transports its bits would each name -- a ruling given in review + of the work for #808 -- so it is looked up as the one whole + value it already is, exactly like any other bare int: refused + when that whole value names no registry, resolved when it happens to equal one instead. Since GitHub issue #860 moved this class off power-of-two spacing, a hand-built composite is no longer guaranteed to be the former -- see @@ -530,11 +532,12 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ # either numbering. A genuine member reaching this point is # ``undefined`` -- the four real transports would already have # resolved above, and :meth:`TransportProtocol.get` cannot mint - # anything else, per this PR's own maintainer ruling against - # extending TransportProtocol at all -- and a bare :class:`int` - # reaches here whenever it matches no real member's value, e.g. a - # stray bit like ``17``. A composite built by hand used to reach - # here just as reliably, since no combination of the old power-of- + # anything else, per the maintainer ruling given in review of the + # work for #808, against extending TransportProtocol at all -- and + # a bare :class:`int` reaches here whenever it matches no real + # member's value, e.g. a stray bit like ``17``. A composite built by + # hand used to reach here just as reliably, since no combination of + # the old power-of- # two bits ever equalled a single real member's value; that is no # longer true under this class's current sequential numbering -- # ``TransportProtocol.tcp | TransportProtocol.udp`` (``3``) is @@ -545,10 +548,10 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[ # and naming every transport whose bit was set -- the fix for # GitHub issue #759, where resolving a composite by picking its # lowest set bit dispatched every one containing ``tcp`` into the - # TCP registry regardless of what else it named. The owner's - # further ruling on this PR (#836) retired that decoding along with - # the rest of the composite handling: "since it's no longer a Flag, - # `|` joined values are no longer parsed and accepted, we will + # TCP registry regardless of what else it named. A further ruling + # given in review of the work for #808 retired that decoding along + # with the rest of the composite handling: "since it's no longer a + # Flag, `|` joined values are no longer parsed and accepted, we will # treat it as a whole, instead of splitting." So a composite's bits # are never decoded looking for a partial answer any more, on # either numbering -- it is looked up as the one whole value it diff --git a/pcapkit/vendor/reg/ethertype.py b/pcapkit/vendor/reg/ethertype.py index 93a5992642..de08a19136 100644 --- a/pcapkit/vendor/reg/ethertype.py +++ b/pcapkit/vendor/reg/ethertype.py @@ -36,7 +36,7 @@ class EtherType(Vendor): #: carrying no real assignment -- a company holding the block but naming #: nothing (``DEC Unassigned``), or a range the list says is dead/invalid #: outright -- rather than a proprietary protocol's real name. The owner's - #: original ruling on #775/#847 held these two out as the only rows to + #: original ruling on #775/#841 held these two out as the only rows to #: convert to :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_ #: member`, on the theory that a proprietary protocol's company name IS #: the final concrete name for every other row. #775's final round diff --git a/pcapkit/vendor/reg/linktype.py b/pcapkit/vendor/reg/linktype.py index ba0fe4ad2f..d0e8064066 100644 --- a/pcapkit/vendor/reg/linktype.py +++ b/pcapkit/vendor/reg/linktype.py @@ -69,7 +69,7 @@ def process(self, data: 'list[Tag]') -> 'tuple[list[str], list[str]]': ] # Value-aware guard for the ``sink`` decision below (GitHub issue #852, - # a follow-up to #848/#844). A row's notes mentioning "legacy" is + # a follow-up to #844). A row's notes mentioning "legacy" is # tcpdump's only textual signal for *which* member of a duplicated # value is the deprecated one, but that word can appear in a row's # notes for an unrelated reason -- e.g. referencing some other, diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index 03588af7cf..5f4fe846be 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -643,21 +643,23 @@ class ConstEnumRegisterFallbackTests(unittest.TestCase): """A once-sanctioned divergence, now retired for these three specific registries. - In the owner's words, the const enums used to mirror the built-in "with - one exception: they contain the missing then register fallback (mutable - enums)". :class:`~pcapkit.const.http.method.Method`, + The owner's design intent, recorded on GitHub issue #647, was for the const + enums to mirror the built-in enum's behaviour with one exception: a missing + value falls back to registering it (the mutable enums). :class:`~pcapkit.const.http.method.Method`, :class:`~pcapkit.const.ftp.command.Command`, :class:`~pcapkit.const.ftp.command.FEATCode` and :class:`~pcapkit.const.reg.apptype.apptype.AppType` (with its four transport subclasses) were, until GitHub issue #860 step 2, the last registries still living that divergence for an *unrecognised* value (every numeric registry had already lost it under #775/#847's ruling). - The owner's #860 ruling retired it for all of them, verbatim: *"I think - we should not mint on get still actually. For all three, only IANA - registered ones are legit values and we need register to properly - create new entries. get will not have sufficient information to create - new ones."* Stated for the three PR 1 converted, but the reasoning is - unconditional and PR 2 applies it to ``AppType`` identically. Concretely, + GitHub issue #860 is where the owner retired it for all of them: + ``get()`` must not mint, because only ``register()`` may create a new + registry entry, and for these registries only IANA-registered values + are legitimate -- ``get()`` does not have enough information to build + one on its own. Stated for :class:`Method`, :class:`Command` and + :class:`FEATCode`, the three registries PR 1 converted, but the + reasoning is unconditional and PR 2 applies it to ``AppType`` + identically. Concretely, :class:`Command` needs :attr:`~pcapkit.const.ftp.command.Command.feat`/ :attr:`~pcapkit.const.ftp.command.Command.desc`/ :attr:`~pcapkit.const.ftp.command.Command.type`/ @@ -848,10 +850,10 @@ def test_transport_protocol_can_no_longer_be_extended_at_runtime(self) -> None: alone: stock ``ad4805f5f`` still mints at ``max_val * 2``, so ``.get('quic')`` there returns ``16``, right after ``dccp``'s ``8``. This PR's own intermediate revision, not #808, is what switched an - unrecognised name to ``max_val + 1`` instead, minting ``9``. PR - #836's own inline comment on ``TransportProtocol.get`` removes the - registration entirely regardless of which scheme numbered it: "Do - not allow extension of TransportProtocol at all." Unlike + unrecognised name to ``max_val + 1`` instead, minting ``9``. The + review ruling on PR #836 removes the registration from + ``TransportProtocol.get`` entirely, regardless of which scheme numbered + it: ``TransportProtocol`` is not to be extended at all. Unlike :class:`~pcapkit.const.ipv4.protection_authority.ProtectionAuthority` and :class:`~pcapkit.const.mh.cga_type.CGAType` below, ``TransportProtocol`` was never one of :data:`EXPECTED_TO_REGISTER` @@ -999,10 +1001,10 @@ def test_self_check_of_the_percent_format_walk(self) -> None: def test_no_percent_formatting_survives_in_the_issue_804_pair(self) -> None: """Neither const module nor either generator still formats with ``%``. - The maintainer's convention from GitHub issue #783: *"id like to keep - f-string convention across the library. only use % substitution when - inevitable."* A ``__repr__`` is not an inevitable case, and neither is - a ``wrap_comment`` argument. + The house convention, set in review on PR #783: + f-strings across the whole library, with ``%`` substitution only where + it is inevitable. A ``__repr__`` is not an inevitable case, and neither + is a ``wrap_comment`` argument. Covers the vendor modules as well as the const ones because the tree is generated: a conversion that is not in the template is reverted by diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index f62d2ccb5a..81409d4c81 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -1,10 +1,10 @@ # -*- coding: utf-8 -*- """Regression tests for GitHub issue #775's tier 1: the miss path must not mint. -The maintainer's ruling on #775, verbatim: *"so that we dont create registered -enums out of unrecognised/unregistered values, unless user/caller explicitly -created them"* -- and lookup never counts as asking for a name, only the new -:meth:`register` classmethod does. Before this change, both ``get()``'s string +The ruling on #775 is that no registry creates a registered enum out of an +unrecognised or unregistered value, however legitimate but unbounded the values +are, unless the user or caller explicitly created it -- and lookup never counts as +asking for a name, only the new :meth:`register` classmethod does. Before this change, both ``get()``'s string path and ``_missing_``'s bounded-but-unassigned range branch called :func:`aenum.extend_enum`, permanently growing the registry for a value nobody asked to be named. Measured on this tree before the fix: @@ -24,10 +24,14 @@ Tier 1 fixed the *mechanism* (a lookup should never mint); it did not decide, registry by registry, which of the remaining ``_missing_`` bodies mint -something worth keeping. That is tier 2, decided on #775 and carried out on -#847: the owner's ruling, verbatim, is *"a final concrete assigned name -> -mint; a notation for the readers -> unmint"*. Applied registry by registry to -every ``_missing_`` that still called :func:`~aenum.extend_enum` -- measured +something worth keeping. That is tier 2; the owner's mint/unmint criterion was +settled on PR #847 and confirmed on #775 as the core of the ruling: a name +is minted when it is the final concrete name an assignment gave, and left +unminted when it is only a notation for the readers. A dynamically or statically +assigned range is as unspecified as any other -- it gets concrete names when +something assigns them -- so its placeholder is notation, not a name worth +registering. Applied registry by registry to every ``_missing_`` that still called +:func:`~aenum.extend_enum` -- measured at exactly 89 modules by an AST walk over ``pcapkit/const/*.py`` (not the "~92" an earlier pass in this programme estimated) -- the ruling converted 82 of them outright (172 branches, :data:`RULING_CONVERTED_REGISTRIES` below), @@ -83,11 +87,12 @@ :class:`~pcapkit.const.http.method.Method` -- 1 ``_missing_`` branch each, initially left minting pending the owner's ruling (this measurement's own report flagged them as genuinely ambiguous under the criterion, since -nothing about them is a manufactured placeholder). The owner's ruling on -#860, verbatim: *"I think we should not mint on get still actually. For all -three, only IANA registered ones are legit values and we need register to -properly create new entries. get will not have sufficient information to -create new ones."* That reasoning reaches ``get()`` as well as +nothing about them is a manufactured placeholder). GitHub issue #860 is +where the owner settled it: ``get()`` must not mint, because only +``register()`` creates a new registry entry -- for these three registries +only IANA-registered values are legitimate, and ``get()`` does not have +enough information to construct one itself. That rule is about ``get()`` +in its own right, and it extends just as much to ``_missing_`` -- :class:`Command` needs ``feat``/``desc``/``type``/``conf`` and :class:`Method` needs ``safe``/``idempotent``, neither of which a bare wire string carries -- so both classes' own ``get()`` (a second, independent @@ -111,42 +116,41 @@ name a real (if IANA-assigned to a whole span rather than declared individually) service rather than a placeholder. -Untouched, per the owner's ruling settling the ``needs: decision`` this PR -re-opened: :class:`~pcapkit.const.ftp.command.CommandType` keeps ``IntFlag``, -verbatim: *"okay, let's keep IntFlag if that's how RFC/IANA data is -constructed (`|` may exist on the CSV data)"* -- measured, 2 occurrences in +Untouched, per the owner's ruling on #860 settling the ``needs: decision`` that +issue re-opened: :class:`~pcapkit.const.ftp.command.CommandType` keeps +``IntFlag``, because that is how the RFC/IANA data is constructed and ``|`` may +appear in the CSV -- measured, 2 occurrences in the generated data join two kinds with ``/`` and would break under a plain ``IntEnum``; see :class:`BespokeOpenVocabularyUnmintConvertedTests`'s own ``test_commandtype_is_untouched``. :class:`~pcapkit.const.reg.apptype.apptype.TransportProtocol`, by contrast, *did* change -- on a different ruling than CommandType's, not the same one. -GitHub PR #836 is what first retired ``|``-composite decoding, verbatim: -*"since it's no longer a Flag, `|` joined values are no longer parsed and -accepted, we will treat it as a whole, instead of splitting"* -- and it was -this issue, #860, that later drew the further consequence once nothing -decoded a composite any more: *"2-power on TransportProtocol is mainly for -the consideration of previously `tcp | udp`-alike code. But we dont accept -this kind of piping anymore in the new logic so I think `auto()` is the -expected behaviour."* So it now numbers its five members sequentially from +GitHub PR #836 is what first retired ``|``-composite decoding: once +``TransportProtocol`` is no longer a ``Flag``, a ``|``-joined value is not parsed +and accepted but treated as a whole, instead of being split. GitHub issue #860 +later drew the further consequence once nothing decoded a composite any more: +the power-of-two spacing existed only for the ``tcp | udp``-style code that +composition served, and since the new logic does not accept that piping, ``auto()`` +is the expected numbering. So it now numbers its five members sequentially from 0 -- ``undefined`` a direct, explicit ``0``, the rest continuing from it via ``auto()`` with no ``_start_`` needed, per the owner's own further ruling settling the declaration shape -- rather than by the power-of-two -spacing that composition used to need; per the owner's own follow-up, -*"no need to add test around -that"* for the one behavioural consequence -- and it is a real consequence, +spacing that composition used to need. The owner also ruled, on #860, that no +test should pin the one behavioural consequence, since it is an obsolete path +left by an accepted breaking change -- and it is a real consequence, not a refusal: a hand-composed ``tcp | udp`` (or a bare, uncomposed ``3``) now silently resolves as ``sctp``'s own value through :meth:`AppType._dispatch`, where it used to name no registry at all and -raise. None of the tests below pin that, per the same instruction. +raise. None of the tests below pin that, per the same ruling. GitHub issue #775's final round closes the two mixed registries themselves: every one of :class:`~pcapkit.const.reg.ethertype.EtherType`'s 52 still- minting range branches, and :class:`~pcapkit.const.ipx.socket.Socket`'s one (``Registered by Xerox``), now convert to :meth:`~pcapkit.corekit.enum. -EnumRegistry._unregistered_member` too -- the owner's ruling, verbatim: -*"Preserve each branch's existing name argument exactly as the current code -produces it -- this change is about not registering, not about renaming -anything."* So each keeps the hex-suffixed name it always rendered +EnumRegistry._unregistered_member` too -- PR #878 scoped the change this way: +preserve each branch's existing name argument exactly as the current code +produces it, since this is about not registering rather than about renaming +anything. So each keeps the hex-suffixed name it always rendered (``Xyplex_0x0888``, not a bare ``Xyplex``) even though it no longer registers -- neither is "mixed" any more, both are wholly converted like the 82 in :data:`RULING_CONVERTED_REGISTRIES`, and :class:`EtherTypeMixedMintTests` @@ -589,10 +593,10 @@ def _first_unassigned_value(cls: 'type') -> 'int': } #: The one probe the original ruling held out as "a real ownership fact, keep #: minting" -- Xyplex, 0x0888. #775's final round converts it too, preserving -#: the hex-suffixed name exactly as the crawler always rendered it (the -#: owner's ruling, verbatim: *"Preserve each branch's existing name argument -#: exactly as the current code produces it -- this change is about not -#: registering, not about renaming anything."*), so this now pins the +#: the hex-suffixed name exactly as the crawler always rendered it (PR #878 +#: scoped it that way: preserving each branch's existing name argument +#: exactly as the current code produces it, since this is about not +#: registering rather than about renaming anything), so this now pins the #: opposite of what its name suggests: that the formerly-kept probe no #: longer mints either. Kept as its own constant, distinct from #: :data:`ETHERTYPE_UNASSIGNED_PROBES`, because :class:`EtherTypeMixedMintTests` @@ -796,10 +800,10 @@ def test_unresolvable_string_key_with_default_in_unassigned_range(self) -> None: pseudo-member the int path does -- it simply does not resolve, and the original key's own error propagates instead. Before #864 this resolved via ``cls(default)`` -> ``_missing_`` to an unregistered - pseudo-member; that is exactly the cost the owner's ruling accepts - ("a default naming a value with no registered member stops - resolving, on every registry... today such a default returns an - unregistered member via _missing_").""" + pseudo-member; that is exactly the cost the owner accepted on #864 in + choosing a value-only lookup: a default naming a value with no + registered member stops resolving, on every registry, where it used + to return an unregistered member via ``_missing_``.""" from pcapkit.const.arp.hardware import Hardware before = len(Hardware.__members__) @@ -1204,15 +1208,18 @@ def _is_hex_suffixed_unregistered_name(name_arg: 'Optional[ast.expr]') -> bool: class UnregisteredMemberNameIsBareTests(unittest.TestCase): - """#775's Q1 follow-up, the maintainer's ruling verbatim: *"Q1 - bare it - is."* Asked whether the non-minting path should honour the registry's - own ``unassigned``/``reserved`` name directly or keep appending the - numeric value, he chose the bare name -- safe precisely because a + """The naming rule settled under GitHub issue #775's tier 1: a non-minting + pseudo-member carries the registry's own ``unassigned``/``reserved`` name + bare, with no numeric value appended. Minted members keep their number, + because there it is part of the real name (``Motorola_0x8705`` names one + EtherType inside Motorola's block). Asked whether the non-minting path should + honour the registry's own name directly or keep appending the numeric value, + the owner chose the bare name -- safe precisely because a pseudo-member built by :meth:`_unregistered_member` never enters - ``__members__``/``_member_map_``/``_value2member_map_``, so two - same-named pseudo-members (e.g. ``Chunk._unregistered_member(20, - 'Unassigned')`` and ``(70, 'Unassigned')``) cannot collide the way two - *minted* ``extend_enum`` members with the same name would. + ``__members__``/``_member_map_``/``_value2member_map_``, so two same-named + pseudo-members (e.g. ``Chunk._unregistered_member(20, 'Unassigned')`` and + ``(70, 'Unassigned')``) cannot collide the way two *minted* + ``extend_enum`` members with the same name would. This walks every generated :mod:`pcapkit.const` module by AST -- rather than pinning one example -- and asserts that every @@ -1285,10 +1292,10 @@ class UnregisteredMemberNameIsBareTests(unittest.TestCase): GitHub issue #775's final round converts the last 53 minting branches -- all 52 of :class:`~pcapkit.const.reg.ethertype.EtherType`'s and :class:`~pcapkit.const.ipx.socket.Socket`'s one -- and deliberately keeps - each one's hex-suffixed name unchanged (the owner's ruling, verbatim: - *"Preserve each branch's existing name argument exactly as the current - code produces it -- this change is about not registering, not about - renaming anything."*). That is the exact manufactured, value-suffixed + each one's hex-suffixed name unchanged (PR #878 scoped it this way: + preserving each branch's existing name argument exactly as the current + code produces it, since this is about not registering rather than + renaming anything). That is the exact manufactured, value-suffixed shape :func:`is_manufactured` exists to flag -- unlike ``'%s_unknown' % namespace`` above, the substituted operand here really is ``value``, via ``hex(value)[2:].upper().zfill(4)``. Flagging it anyway would be a false @@ -1535,10 +1542,10 @@ def test_formerly_attributed_vendor_block_no_longer_mints(self) -> None: exact probe (``Xyplex``, 0x0888) used to prove the ruling *kept* minting a real attributed name; GitHub issue #775's final round converts it, preserving the hex-suffixed name exactly as the crawler - always rendered it -- the owner's ruling, verbatim: *"Preserve each + always rendered it -- PR #878 scoped it this way: preserve each branch's existing name argument exactly as the current code - produces it -- this change is about not registering, not about - renaming anything."* Same shape as + produces it, since this is about not registering rather than + renaming anything. Same shape as :meth:`test_unassigned_rows_do_not_mint` above, just for the one probe that used to be the exception.""" from pcapkit.const.reg.ethertype import EtherType @@ -1871,12 +1878,11 @@ class BespokeOpenVocabularyUnmintConvertedTests(unittest.TestCase): (``Unassigned_%d``, ``Unknown_%d``); each minted the literal, exact string it was asked to resolve, as its own name. That initially read as a case for keeping them minting (the label was never manufactured), but - the owner's ruling on #860 settled it the other way, verbatim: *"I think - we should not mint on get still actually. For all three, only IANA - registered ones are legit values and we need register to properly - create new entries. get will not have sufficient information to create - new ones."* Concretely: :class:`~pcapkit.const.ftp.command.Command` - needs ``feat``/``desc``/``type``/``conf`` and + GitHub issue #860 settled it the other way: ``get()`` does not mint, + because only ``register()`` creates a new registry entry, and for these + registries only IANA-registered values are legitimate -- ``get()`` + simply does not have enough information to build a new one. Concretely: + :class:`~pcapkit.const.ftp.command.Command` needs ``feat``/``desc``/``type``/``conf`` and :class:`~pcapkit.const.http.method.Method` needs ``safe``/``idempotent``, neither of which a bare wire string carries, so minting used to register a permanently hollowed-out member for each. @@ -1921,11 +1927,10 @@ def test_featcode_import_mints_nothing(self) -> None: so every :class:`Command` row references one by plain attribute access and nothing is minted merely by importing the module. - Deliberately not a literal member count: the owner's own words, - *"this might break when IANA updated their list. i dont like this - guard on the test cases"* -- a regeneration that correctly picks up - a newly-registered FEAT code would fail a hardcoded number for being - *right*. The invariant that survives a table update instead: every + Deliberately not a literal member count: the owner ruled against + that guard on #860, because a hardcoded number breaks whenever IANA + updates its list -- a regeneration that correctly picks up + a newly-registered FEAT code would fail it for being *right*. The invariant that survives a table update instead: every name in ``__members__`` is a real declaration in the generated source, not something built by a call at import time. A regeneration moves declarations and members together; only an @@ -2141,10 +2146,10 @@ def test_all_three_carry_the_registry_protocol(self) -> None: self.assertTrue(callable(getattr(cls, 'register_alias', None))) def test_commandtype_is_untouched(self) -> None: - """The owner's ruling also floated ``CommandType`` -> ``IntEnum``. - Re-opened as `needs: decision` on #860 and settled the other way, - verbatim: *"okay, let's keep IntFlag if that's how RFC/IANA data is - constructed (`|` may exist on the CSV data)."* Measured: 2 occurrences + """``CommandType`` -> ``IntEnum`` was floated too. + Re-opened as `needs: decision` on #860 and settled the other way: it + keeps ``IntFlag``, because that is how the RFC/IANA data is constructed + and ``|`` may appear in the CSV. Measured: 2 occurrences in the generated data join two kinds with ``/``, e.g. access *and* parameter, which a plain ``IntEnum`` cannot represent. Pinned here so a future change is noticed as a scope change rather than folded @@ -2361,9 +2366,11 @@ class AppTypeUnmintConvertedTests(unittest.TestCase): merely being imported, which does not exist here, since every one of these 766 spans mints only on an actual port lookup -- and the owner's ruling for the whole ``AppType`` family draws no distinction between a - real name and a placeholder: *"only IANA registered ones are legit - values and we need register to properly create new entries. get will - not have sufficient information to create new ones."* A lookup resolving + real name and a placeholder: GitHub issue #860 settled that ``get()`` + must not mint regardless of whether the value it would construct is a + genuine IANA-registered name or a manufactured placeholder, because + only ``register()`` may create a new entry and ``get()`` never has + enough information to build one itself. A lookup resolving ``TCP(6010)`` after this PR therefore returns an *unregistered* ``x11`` member -- correct as a service name, but absent from ``__members__``/``_value2member_map_`` until someone calls @@ -2642,15 +2649,14 @@ def test_apptype_unrecognised_proto_is_still_refused(self) -> None: class TransportProtocolAutoTests(unittest.TestCase): """:class:`~pcapkit.const.reg.apptype.apptype.TransportProtocol`'s - ``auto()`` conversion, #860 step 2 PR 2's other change. The owner's - ruling, verbatim: *"since we do not allow `|` anymore, using `auto()` - instead of 2-power is the right move (breaking change but accepted)."* - And, on the one behavioural consequence -- a hand-composed + ``auto()`` conversion, #860 step 2 PR 2's other change. The owner ruled on + #860 that, since ``|`` composition is no longer allowed, ``auto()`` in place + of power-of-two spacing is the right move -- a breaking change, but an + accepted one. On the one behavioural consequence -- a hand-composed ``tcp | udp`` now equalling ``sctp`` numerically (``1 | 2 == 3``), where it - used to name no member at all and be refused as a whole -- the explicit - follow-up settling that no test should pin it: *"No need to add test - around that honestly. This is an obsoleted path from a breaking - change."* So this class deliberately covers only the values and the + used to name no member at all and be refused as a whole -- the same thread + settled that no test should pin it: the path is obsolete after the breaking + change, and a test would preserve it as though it still mattered. So this class deliberately covers only the values and the stale comment's removal, not the composition consequence: no test here calls ``_dispatch`` with a composed value at all, unlike :meth:`AppTypeUnmintConvertedTests. @@ -2693,9 +2699,9 @@ def test_get_still_refuses_an_unrecognised_name(self) -> None: TransportProtocol.get('not-a-real-transport') def test_stale_power_of_two_comment_is_gone(self) -> None: - """The owner's instruction, verbatim: the in-code comment claiming - the values "must keep" power-of-two spacing contradicted GitHub - PR #836's own ruling once nothing decomposed a composite any + """Per GitHub issue #860: the in-code comment claiming + the values "must keep" power-of-two spacing contradicted the PR #836 + ruling that nothing composes a ``TransportProtocol`` any more, and is deleted rather than merely superseded -- checked against the generated source itself, not the docstring here, so a regeneration that reintroduces it fails this test.""" diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index ddd959e935..5f0d75c3d4 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -7,11 +7,11 @@ their own, because each of those carries a hand-copied ``get()`` -- and none of them carries ``register``, ``register_alias`` or ``get_all`` at all. -The maintainer's ruling on #842, verbatim: *"to finalise the abstraction idea, -get/get_all/register/register_alias should always exist on the const enums - so -they're to be moved to the base class. And AppType's sub-base class will do its -necessary overrides and dispatching logic; AppType subclasses will have their -necessary overrides again pertaining their different contracts."* +The design ruling on #842 finalises the abstraction: ``get``, ``get_all``, +``register`` and ``register_alias`` should always exist on the const enums, so they +move to the base class and every registry answers the same four calls. The +``AppType`` sub-base class keeps its own overrides and dispatching logic, and its +subclasses override further where their contracts differ. :class:`~pcapkit.corekit.enum.EnumRegistry` is tier one of that hierarchy. This module pins both halves of the claim: that the generated registries in this batch @@ -322,7 +322,10 @@ def test_tcp_flags_gains_the_protocol_it_never_had(self) -> None: class GetContractTests(unittest.TestCase): - """*"get is a shortcut for ``[]`` operation and returns the canonical enum."*""" + """``get`` is a shortcut for the ``[]`` operation and returns the canonical enum. + + The contract set on #842. + """ def setUp(self) -> None: snapshot = snapshot_modules(ISOLATED_PREFIXES) @@ -397,7 +400,10 @@ def test_get_never_mints_on_any_converted_registry(self) -> None: class GetAllContractTests(unittest.TestCase): - """*"get_all returns all matching enums."*""" + """``get_all`` returns every matching enum, not only the canonical one. + + The contract set on #842. + """ def setUp(self) -> None: snapshot = snapshot_modules(ISOLATED_PREFIXES) @@ -434,7 +440,11 @@ def test_get_all_propagates_a_miss(self) -> None: class RegisterContractTests(unittest.TestCase): - """*"register mints new enum to the class at runtime with specified names."*""" + """``register`` mints a new enum on the class at runtime under the names it is given. + + The contract set on #842: the caller supplies the names, so nothing has to guess + one blindly. + """ def setUp(self) -> None: snapshot = snapshot_modules(ISOLATED_PREFIXES) @@ -568,10 +578,13 @@ def test_register_alias_still_aliases_after_the_value_guard(self) -> None: class RegisterAliasContractTests(unittest.TestCase): - """*"register_alias(es) adds additional alias(es) to a given enum's mapping."* + """``register_alias`` adds additional alias(es) to a given enum's mapping. - And, on whether an enum must be given: *"actually i think it should always be - for an existing member"*. + The contract set on #842, which also ruled that an alias is always for an + existing member, unless ``AppType`` (and the concrete enums) must call it on a + member that does not exist yet. One method name then carries one convention + across the registries, and for the plain registries an alias is a caller-opt-in + name that IANA does not record. """ @@ -1360,10 +1373,15 @@ def test_type_is_the_dedicated_sentinel_class_and_only_the_object_is_exported(se """GitHub issue #911 reversed half of what this used to assert. It read ``assertIn('NoDefaultType', enum_module.__all__)`` -- the type - *and* the object were exported. The owner's ruling: *"we should ONLY - export the objects (like* ``NULL`` *) to users"*, so the type is out of - :attr:`__all__` while staying importable by its dotted path, which is - what the last assertion here pins. + *and* the object were exported. GitHub issue #719 is where the owner + settled the rule behind this: a module's ``__all__`` lists a + sentinel's object (such as ``NO_DEFAULT``), and deliberately leaves + its type (such as ``NoDefaultType``) out, because the type is not + part of the public surface -- exposing only the final object is what + keeps the published API to what a caller actually uses. GitHub issue + #911 is the issue that ruling's implementing work belongs to. The + type is out of :attr:`__all__` while staying importable by its + dotted path, which is what the last assertion here pins. """ import pcapkit.corekit.enum as enum_module @@ -1494,13 +1512,16 @@ class GetDefaultNoMintTests(unittest.TestCase): and the value branch's -- reached ``_missing_`` for a ``default`` that fell inside a still-minting registry's own range, growing the registry as a side effect of resolving ``default`` rather than ``key``. The - owner's ruling, verbatim: *"Take (b). Only register can mint. get should - not mint unless it falls through the ``_missing_``'s minted ranges."*, - and on the implementation, choosing option 1 of three: *"I think 1 is - correct mechanism we'd like."* -- ``default`` now resolves through a - plain ``_value2member_map_`` lookup only, so it cannot mint by - construction, while ``key`` resolution -- and whatever it lets - ``_missing_`` do -- is deliberately unchanged. + owner ruled on #864 that only ``register`` may mint: ``get`` mints only + as the natural outcome of ``key`` falling through one of ``_missing_``'s + minted ranges, never because a fallback happened to resolve. Of the + three mechanisms offered, the owner chose the one that cannot mint by + construction -- ``default`` now resolves through a plain + ``_value2member_map_`` lookup only, with no snapshot-and-undo and no + per-class marker to keep in step with future crawlers -- at the price of a + ``default`` in a declared-but-unassigned range no longer resolving. + ``key`` resolution -- and whatever it lets ``_missing_`` do -- is + deliberately unchanged. """ def setUp(self) -> None: @@ -1546,10 +1567,10 @@ def test_the_issues_own_case_no_longer_mints_and_raises_instead(self) -> None: def test_key_path_minting_is_preserved(self) -> None: """Originally a no-change guard: ``EtherType.get(0x0888)`` -- no ``default`` at all -- used to mint ``Xyplex_0x0888`` through its own - ``_missing_``, back when ruling one's mint/unmint criterion - (*"get should not mint unless it falls through the _missing_'s - minted ranges"*) still classed ``Xyplex`` as a kept-minting, - real-attributed-name range. GitHub issue #775's final round converts + ``_missing_``, back when #775's original mint/unmint criterion still + classed ``Xyplex`` as a kept-minting, real-attributed-name range -- + the one kind of mint #864 ruled ``get`` may still produce, by falling + through ``_missing_``'s minted ranges. GitHub issue #775's final round converts that range (and every other one still minting on ``EtherType``/ :class:`~pcapkit.const.ipx.socket.Socket`) to :meth:`~pcapkit.corekit. enum.EnumRegistry._unregistered_member`, preserving the hex-suffixed diff --git a/tests/corekit/test_enum_lookup_reparent_930_unit.py b/tests/corekit/test_enum_lookup_reparent_930_unit.py index 34973fcb23..745408d008 100644 --- a/tests/corekit/test_enum_lookup_reparent_930_unit.py +++ b/tests/corekit/test_enum_lookup_reparent_930_unit.py @@ -27,10 +27,10 @@ ``mypy``'s ``[override]`` check plus ``pylint``'s ``arguments-differ`` both flagged the resulting shape mismatch, silenced with a suppression. GitHub issue #935 first answered that on the owner's ruling, verbatim: *"I lean on 1"* -- widen both signatures to accept -``default`` and delete the suppression. Asked, on the same issue, *"why must we have the -two overrides tho? cant they directly fall back to the base class's?"*, the owner's final -ruling went further, verbatim: *"I prefer (2) directly"* -- deleting both overrides -outright rather than widening them. +``default`` and delete the suppression. Asked, on GitHub pull request #940 -- which was +implementing that widening -- *"why must we have the two overrides tho? cant they directly +fall back to the base class's?"*, the owner's final ruling went further, verbatim: *"I +prefer (2) directly"* -- deleting both overrides outright rather than widening them. Measured before acting on that final ruling: neither override ever minted an alias -- ``__members__`` and ``list(cls)`` agree at 6 and 4 -- so what each docstring called @@ -198,9 +198,9 @@ def test_esp_status(self) -> None: self.assertEqual(len(list(ESPStatus)), 6) def test_fast_binding_acknowledgment_status(self) -> None: - """GitHub issue #935 later deleted its kept ``get`` override outright - (the owner's ruling, verbatim: *"I prefer (2) directly"*), so the - decorator this once pinned no longer exists to pin -- + """GitHub pull request #940 later deleted its kept ``get`` override + outright (the owner's ruling, verbatim: *"I prefer (2) directly"*), so + the decorator this once pinned no longer exists to pin -- :class:`AllSevenInheritTheBareClassmethodTests` now covers this class alongside the other six.""" from aenum import IntEnum @@ -450,10 +450,10 @@ class was ``KeptOverrideQuietnessTests`` while both classes still carried their own ``get``, first through #930's re-parenting and briefly again through GitHub issue #935's first attempt, which widened that override to accept ``default`` rather than delete it. The owner's - final ruling on #935 deleted both outright instead, verbatim: *"I - prefer (2) directly"*. What this class pins did not change with that - deletion -- the quiet raise -- only *how* it is produced: through - :meth:`~pcapkit.corekit.enum.EnumLookup.get` + final ruling, given on GitHub pull request #940, deleted both outright + instead, verbatim: *"I prefer (2) directly"*. What this class pins did not + change with that deletion -- the quiet raise -- only *how* it is produced + through :meth:`~pcapkit.corekit.enum.EnumLookup.get` (:mod:`pcapkit.corekit.enum`) directly now, rather than through an override that reconciled itself onto the base's shape. @@ -573,12 +573,12 @@ class AllSevenInheritTheBareClassmethodTests(unittest.TestCase): Five were always this way -- pure re-parents, having defined no ``get`` of their own to begin with. The other two, :class:`FastBindingAcknowledgmentStatus` and - :class:`IPv6AddressPrefixCode`, joined them only at GitHub issue #935's + :class:`IPv6AddressPrefixCode`, joined them only at GitHub pull request #940's final revision: their own kept ``get`` stayed a :class:`staticmethod` - through #930's re-parenting and briefly again through #935's first - attempt (which widened it to accept ``default`` rather than delete it), - and only the owner's final ruling there -- verbatim, *"I prefer (2) - directly"* -- deleted it outright, collapsing the seven-way split this + through #930's re-parenting and briefly again through #935's first attempt + (which widened it to accept ``default`` rather than delete it), and only + the owner's final ruling on #940 -- outright deletion over widening the + signature -- removed it, collapsing the seven-way split this class used to test as five-plus-two into one uniform case. This class was named for the five alone before that ruling, and :class:`ReparentedBasesTests` pinned the other two's surviving diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py index 241a1a8f9e..f78aaedb5a 100644 --- a/tests/corekit/test_sentinel_exports_unit.py +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -1,9 +1,14 @@ # -*- coding: utf-8 -*- """GitHub issue #911: a sentinel exports its **object**, never its type. -The owner's ruling, verbatim: *"One thing about sentinel types and objects in the -library: we should ONLY export the objects (like* ``NULL`` *) to users."* That is -one rule with two directions, and the tree on ``origin/main`` broke it in both: +GitHub issue #719 is where the owner settled this rule: a module's +``__all__`` lists a sentinel's object (such as ``NULL``), and deliberately +leaves its type (such as ``NullType``) out. The reason is the context a +bare rule would not give: the types are not part of the public surface, so +exposing only the final objects is what keeps the published API to the +things a caller actually uses. GitHub issue #911 is the issue the +implementing work for that ruling belongs to. That is one rule with two +directions, and the tree on ``origin/main`` broke it in both: * ``pcapkit.corekit.module.__all__`` was ``['NULL', 'NullType', 'ModuleDescriptor']`` -- the type is exported; @@ -31,11 +36,16 @@ ``_Absent``/``_AbsentType``, with a leading underscore. :class:`SentinelPopulationTests` pins the count and the doc together, so the next sentinel cannot be added to one without the other. ``ABSENT`` is private and stays out of :attr:`__all__` in both -directions, which is what the ruling means by "to users". +directions -- not under the export ruling, which argues the opposite for an +object like this one, but under the privacy ruling stated below, which treats a +sentinel documented as private as having no place in :attr:`__all__` at all. A follow-up to this same issue moved all four *definitions* into -:mod:`pcapkit.corekit.sentinels`, per the owner's later ruling -- *"Okay one module -for all four it is."* Every assertion above still holds unchanged, since it is about +:mod:`pcapkit.corekit.sentinels`: the owner chose one shared module over one module +per sentinel, on #911. The shared module is where the ``Type`` naming +convention and the rules for when to add ``__bool__``, ``__copy__`` or ``__reduce__`` +can sit together, which four near-empty files would scatter. +Every assertion above still holds unchanged, since it is about each original module's ``__all__``, which the re-export shims left untouched; what changed is only :attr:`type.__module__` for the four types, which :meth:`SentinelExportTests.test_every_sentinel_type_is_still_importable_by_name` now @@ -46,9 +56,9 @@ ``NoValue`` to ``NO_VALUE`` and ``_Absent`` to ``ABSENT``, the latter also dropping its leading underscore -- so every instance name agrees on one casing. Privacy for what is now ``ABSENT`` stopped being signalled by the name at all and became -documentation-only, per the owner's ruling on #937: *"we can change* ``_ABSENT`` *to* -``ABSENT`` *just document it as private type/class in the documentation and not for -public use is enough."* +documentation-only. GitHub issue #719 is where the owner ruled that the underscore +can go, because documenting the object and its type as private, and not for public +use, is enough on its own to carry the privacy. :meth:`SentinelExportTests.test_the_private_sentinel_is_exported_neither_way` is what now pins that privacy under the new name, since the mechanical underscore signal it used to double-check is gone. @@ -119,8 +129,8 @@ #: Repository root, for the two tests that read a file rather than import it. ROOT = pathlib.Path(__file__).resolve().parents[2] -#: Where all four sentinels are now *defined*, since GitHub issue #911's housing -#: move -- *"Okay one module for all four it is."* Each entry in :data:`SENTINELS` +#: Where all four sentinels are now *defined*: the single shared module GitHub +#: issue #911 chose over one module per sentinel. Each entry in :data:`SENTINELS` #: below used to name a different module here (``pcapkit.corekit.module``, #: ``pcapkit.corekit.fields.field``, ``pcapkit.corekit.enum`` and #: ``pcapkit.protocols.protocol`` respectively); all four now report this one. @@ -317,7 +327,11 @@ class SentinelPopulationTests(unittest.TestCase): """There are four, they follow the naming rule, and the docs say so.""" def test_every_sentinel_follows_the_naming_convention(self) -> 'None': - """*"Keep the sentinel object's type class naming as* ``Type``*."*""" + """A sentinel object's type class is named ``Type``. + + The house convention set under GitHub issue #857, kept uniform so that a + future maintainer adding a sentinel can derive the type's name mechanically. + """ for name, _, type_ in SENTINELS: with self.subTest(sentinel=name): self.assertEqual(type_.__name__, _expected_type_name(name)) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index be3cbe59f9..15eb55c7a2 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -1618,16 +1618,16 @@ def test_mh_get_default_works_uniformly_through_the_inherited_method(self) -> No ``TypeError: get() takes 1 positional argument but 2 were given``. GitHub issue #935's first ruling, verbatim *"I lean on 1"*, widened both signatures to accept ``default`` rather than delete them; asked - next *"why must we have the two overrides tho? cant they directly - fall back to the base class's?"*, the owner's final ruling went - further, verbatim: *"I prefer (2) directly"* -- deleting both - overrides outright. Both classes now inherit ``get`` from the base - exactly as :class:`LMAAddressCode` and :class:`LocalizedRoutingStatus` - -- the two pure re-parents already in this module -- always have, so - ``default`` now works the same way on all four, uniformly, because - there is only one implementation left to call. This test fails with - the ``TypeError`` above against the tree at 382375811, before either - of #935's rulings landed. + next, on GitHub pull request #940, *"why must we have the two + overrides tho? cant they directly fall back to the base class's?"*, + the owner's final ruling there went further, verbatim: *"I prefer (2) + directly"* -- deleting both overrides outright. Both classes now + inherit ``get`` from the base exactly as :class:`LMAAddressCode` and + :class:`LocalizedRoutingStatus` -- the two pure re-parents already in + this module -- always have, so ``default`` now works the same way on + all four, uniformly, because there is only one implementation left to + call. This test fails with the ``TypeError`` above against the tree at + 382375811, before either ruling landed. """ import sys diff --git a/tests/vendor/test_ipx_socket_unit.py b/tests/vendor/test_ipx_socket_unit.py index b502851766..d05ea035de 100644 --- a/tests/vendor/test_ipx_socket_unit.py +++ b/tests/vendor/test_ipx_socket_unit.py @@ -121,10 +121,11 @@ #: numeric suffix. ``Registered by Xerox`` was the one that stayed at the #: time, on the theory that it states a real ownership fact rather than a #: status placeholder -- #775's *final* round converts it too, keeping its -#: hex-suffixed name unchanged (the owner's ruling: preserve the existing -#: name argument exactly, this is about not registering, not about renaming), -#: so all five ranges resolve through :meth:`~pcapkit.corekit.enum. -#: EnumRegistry._unregistered_member` now and none of them mint. +#: hex-suffixed name unchanged -- PR #878 scoped the change this way: +#: preserve the existing name argument exactly, since this is about not +#: registering rather than about renaming -- so all five ranges resolve +#: through :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` +#: now and none of them mint. EXPECTED_MISSING_NAMES = { 0x0000: 'Unspecified', # a defined member 0x0001: 'Routing_Information_Packet', # a defined member diff --git a/tests/vendor/test_vendor_reg_apptype_generator_unit.py b/tests/vendor/test_vendor_reg_apptype_generator_unit.py index ff777a2012..335300be13 100644 --- a/tests/vendor/test_vendor_reg_apptype_generator_unit.py +++ b/tests/vendor/test_vendor_reg_apptype_generator_unit.py @@ -61,9 +61,9 @@ with no bare literal left anywhere, no member's wrapper was load-bearing against a mypy error any more, since an ``auto()``-valued member infers as ``Any``, which is assignable to ``TransportProtocol`` with no cast at all. -The owner's final ruling on this issue reinstated the original shape -instead, verbatim: *"undefined direct uses 0. then other real transport -use auto. so we don't have to define a _start_ and the undefined +The owner's final ruling, given on GitHub pull request #874, reinstated the +original shape instead, verbatim: *"undefined direct uses 0. then other real +transport use auto. so we don't have to define a _start_ and the undefined declaration is explicit."* So ``undefined`` is a direct, explicit ``0`` again, and ``tcp``/``udp``/``sctp``/``dccp`` continue from it via plain ``auto()`` with no ``_start_`` needed at all -- ``auto()`` picks up the @@ -225,13 +225,13 @@ def test_undefined_member_is_cast_rather_than_a_bare_literal(self) -> None: literal left anywhere, no member's wrapper was load-bearing against a mypy error any more (measured at the time: unwrapping ``undefined`` alone stayed mypy-clean, identically to unwrapping ``tcp`` instead). - The owner's final ruling on the issue reinstated the original - shape, verbatim: *"undefined direct uses 0. then other real - transport use auto. so we don't have to define a _start_ and the - undefined declaration is explicit."* So ``undefined`` is a direct, - explicit ``cast('TransportProtocol', 0)`` again, exactly as #770 - first shaped it, and this test's own check (and #770's asymmetry) - are both back to describing the tree as it actually ships. + The owner's final ruling, given on GitHub pull request #874, + reinstated the original shape, verbatim: *"undefined direct uses 0. + then other real transport use auto. so we don't have to define a + _start_ and the undefined declaration is explicit."* So ``undefined`` + is a direct, explicit ``cast('TransportProtocol', 0)`` again, exactly + as #770 first shaped it, and this test's own check (and #770's + asymmetry) are both back to describing the tree as it actually ships. Measured directly, to prove the load-bearing claim rather than assert it: with ``undefined`` unwrapped to a bare ``0`` (everything diff --git a/tests/vendor/test_vendor_snapshot_restore_unit.py b/tests/vendor/test_vendor_snapshot_restore_unit.py index 60cfb15066..05364d384a 100644 --- a/tests/vendor/test_vendor_snapshot_restore_unit.py +++ b/tests/vendor/test_vendor_snapshot_restore_unit.py @@ -4,11 +4,11 @@ GitHub issue #872. Rounds 4-8 made :meth:`~pcapkit.vendor.default.Vendor.__init__`'s own write atomic -- a temp-file-then-:func:`os.replace` at the point of the write, with permission -matching for the replacement. The owner's ruling on that, verbatim: *"I -prefer we use contextlib over manually manage the temp file deletion based -on pure best intent (try-finally) and for atomic writing, an easier path is -simply keep a copy before running the sub-vendor and revert if anything -failed."* +matching for the replacement. The owner's ruling on GitHub pull request +#873, verbatim: *"I prefer we use contextlib over manually manage the temp +file deletion based on pure best intent (try-finally) and for atomic +writing, an easier path is simply keep a copy before running the sub-vendor +and revert if anything failed."* That supersedes the atomic write entirely rather than adjusting it. :meth:`~pcapkit.vendor.default.Vendor._write_atomic` no longer exists;