diff --git a/docs/source/conventions.rst b/docs/source/conventions.rst index 697db867fe..d684c2d5fc 100644 --- a/docs/source/conventions.rst +++ b/docs/source/conventions.rst @@ -227,8 +227,8 @@ inconsistencies**: Where the registry protocol lives --------------------------------- -:meth:`~pcapkit.corekit.enum.EnumRegistry.get`, -:meth:`~pcapkit.corekit.enum.EnumRegistry.get_all`, +:meth:`~pcapkit.corekit.enum.EnumLookup.get`, +:meth:`~pcapkit.corekit.enum.EnumLookup.get_all`, :meth:`~pcapkit.corekit.enum.EnumRegistry.register` and :meth:`~pcapkit.corekit.enum.EnumRegistry.register_alias` are expected to exist on **every** registry, per the ruling on @@ -245,12 +245,118 @@ A handful of registries define their own ``__new__`` to carry extra attributes a do not share the generated template; bringing them onto the base is tracked in `#860 `__. +The Two Tiers, and What Lives on Each +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +:class:`~pcapkit.corekit.enum.EnumRegistry` is not the only base any more. Since +phase 1 of `#877 `__ it has a +parent, and the line between them is whether the enumeration may *grow*: + +=================================================== ============================================================== +:class:`~pcapkit.corekit.enum.EnumLookup` ``get``, ``get_all``, ``_validate_value`` +:class:`~pcapkit.corekit.enum.EnumRegistry` ``register``, ``register_alias``, ``register_aliases``, + ``_extend``, ``_unregistered_member`` +=================================================== ============================================================== + +The owner's ruling, verbatim: *"they may subclass a bare base enum from +pcapkit.corekit.enum - where EnumRegistry subclasses it for using in the other +mutable ones."* So a **closed** set inherits :class:`~pcapkit.corekit.enum.EnumLookup` +directly and is never handed a ``register`` it would have to refuse; an **open** +registry inherits :class:`~pcapkit.corekit.enum.EnumRegistry` exactly as before. + +What settled the split is the owner's own second thought about carrying ``register`` +on the base: *"if it carries ``register``, then why not ``register_alias``. We might +be creating a bad ruling."* Following that through leaves +:class:`~pcapkit.corekit.enum.EnumRegistry` holding only three methods, too thin to +justify a second class -- so the two tiers collapse into one, which is the opposite of +what was ruled. + +**Both tiers are plain classes, and that is load-bearing.** Inserting a parent above +:class:`~pcapkit.corekit.enum.EnumRegistry` leaves the member data type exactly where +it was -- ``LinkType -> EnumRegistry -> EnumLookup -> IntEnum -> int`` -- so +``_member_type_`` still comes from the enum base. Had either tier subclassed +:class:`~aenum.Enum` in order to "be an enum", it would have become the member type +itself and broken ``int``, ``str`` and flag registries at once. + +:meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` is what the base carries +*instead* of ``register``, and it answers the owner's other requirement: *"there must +be some sort of range validation logic for the inherited classes to hook in."* The +base implementation accepts everything; an override states a range, in the shape the +generated registries currently spell by hand in ``_missing_``: + +.. code-block:: python + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + if not (isinstance(value, int) and 0 <= value <= 0xFF): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + +Three things about it are easy to get wrong: + +* **It guards, it does not convert.** The return type is :obj:`None` deliberately, so + that an override cannot normalise a value on its way through and silently change + what a lookup resolves to. +* **Raise from** :mod:`pcapkit.utilities.exceptions`. + :exc:`~pcapkit.utilities.exceptions.EnumValueError` is the fitting one, and because + it subclasses :exc:`ValueError` a rejection is caught by ``get``'s own ``except`` and + falls back to ``default`` like any other unresolvable value. An override raising + outside that hierarchy propagates past ``default`` instead. +* **A** ``str`` **key never reaches it.** That path never calls ``cls(key)``, so it + resolves only against already-populated lookup tables, where every value present is + legal by construction. ``register`` does call it, after its duplicate check. + +.. note:: + + Re-parenting the remaining helper enumerations onto + :class:`~pcapkit.corekit.enum.EnumLookup` is **phase 2** of + `#877 `__ and has not happened + yet. Phase 1 is deliberately behaviour-preserving on its own, which is what let it + land while other work was still in flight on the files the re-parent touches. + +Case Sensitivity Is RFC-Directed +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The rule, in the owner's own wording on +`#877 `__: + + if RFC states the values are case-insensitive, then our enum should also treat them + that way. otherwise, we should treat them case sensitive. + +And the reason a registry's spelling is never quietly normalised, from the same thread: +*"enum should honour and keep their original writings as in the registrars. case +in-sensitivity only applies to certain selected ones, where logically it makes sense +(like ``TransportProtocol``) and/or RFC documentation itself recognises them as +case-insensitive (like, maybe, FTP/HTTP commands)."* + +So :meth:`~pcapkit.corekit.enum.EnumLookup.get` is **case-sensitive**, and that is the +default every enumeration gets. Case-insensitivity is a per-class ``get`` override that +has to cite the RFC or IANA registry making the values case-insensitive; without one it +is a defect rather than a convenience. That also means no public member is ever renamed +to make a lookup work -- which is what keeps +:class:`~pcapkit.const.hip.parameter.Parameter`'s ``R1_Counter = 128`` and +``R1_COUNTER = 129``, two IANA-registered HIP parameters differing only in case, both +resolvable. + +.. note:: + + Auditing every registry's existing case behaviour against the specification its + values come from is `#903 `__, + not something this page records per class. The owner's scope for it, verbatim: + *"we should audit all registries and then decide if case (in)sensitive."* Two + findings already have their own issues -- + :class:`~pcapkit.const.ftp.command.Command`'s ``value.upper()`` is backed by + :rfc:`959#section-4.1` (*"Upper and lower case alphabetic characters are to be + treated identically"*), while + :class:`~pcapkit.const.http.method.Method`'s ``key.upper()`` contradicts + :rfc:`9110#section-9.1`, which makes the method token case-sensitive, and is tracked + as `#896 `__. + .. note:: The obstacle this page used to record -- that the base's string-key path does not fall through to a value lookup, so a :class:`~aenum.StrEnum` registry would stop resolving a valid value that is not also a name -- **no longer applies.** - :meth:`~pcapkit.corekit.enum.EnumRegistry.get` now checks ``_value2member_map_`` + :meth:`~pcapkit.corekit.enum.EnumLookup.get` now checks ``_value2member_map_`` when the name lookup misses, so such a value resolves: .. code-block:: pycon diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index a72b96cf4d..0e8ae2b21d 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -4,15 +4,55 @@ .. module:: pcapkit.corekit.enum -:mod:`pcapkit.corekit.enum` contains :class:`~pcapkit.corekit.enum.EnumRegistry` -only, the base class every constant enumeration under :mod:`pcapkit.const` is to -inherit the registry protocol from. - -The maintainer's ruling on GitHub issue #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."* +:mod:`pcapkit.corekit.enum` contains the two bases every enumeration in this +library is meant to inherit from, split by whether the enumeration may *grow*: + +* :class:`EnumLookup` -- the bare **lookup** half: :meth:`~EnumLookup.get`, + :meth:`~EnumLookup.get_all` and the overridable + :meth:`~EnumLookup._validate_value` guard. Nothing here mutates the + enumeration, so it is what a *closed* set can inherit without being handed a + contract it must refuse. +* :class:`EnumRegistry`, a subclass of the above -- adds the **mutating** half: + :meth:`~EnumRegistry.register`, :meth:`~EnumRegistry.register_alias`, + :meth:`~EnumRegistry.register_aliases`, :meth:`~EnumRegistry._extend` and + :meth:`~EnumRegistry._unregistered_member`. Every generated enumeration under + :mod:`pcapkit.const` inherits from here. + +That split is the owner's ruling on GitHub issue #877, verbatim: *"My initial +thought was to make them immutable - unless RFC/IANA says otherwise. Therefore +they may subclass a bare base enum from pcapkit.corekit.enum - where +EnumRegistry subclasses it for using in the other mutable ones."* + +Which methods land on which tier was settled in the same thread. The owner's own +second thought is what drew the line: *"if it carries ``register``, then why not +``register_alias``. We might be creating a bad ruling."* Following that through, +a base holding both would leave :class:`EnumRegistry` with only +``register_aliases``, ``_extend`` and ``_unregistered_member`` -- too thin to +justify a second class, collapsing the two tiers into one. So all five mutating +methods stay put, and what the base carries instead is the owner's other requirement, +verbatim: *"there must be some sort of range validation logic for the inherited +classes to hook in"* -- which is :meth:`EnumLookup._validate_value`. Legality is +every enumeration's concern; mutation is only the open registries'. + +Supporting measurement, taken on this tree at the time of the split: **0** of the +26 non-registry enumerations define ``register``, ``register_alias``, +``register_aliases`` or ``_unregistered_member``, and there are **0** +:class:`EnumRegistry` subclasses outside :mod:`pcapkit.const`. The mutating half +therefore had no users to serve among the classes being re-parented. + +.. note:: + + Re-parenting the remaining helper enumerations onto :class:`EnumLookup` is + **phase 2** of GitHub issue #877 and has not happened yet: introducing the + base is 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. + +The registry tier's own shape is the earlier ruling on GitHub issue #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."* That is a three-tier hierarchy, of which this module is **tier one**: @@ -57,12 +97,12 @@ class will do its necessary overrides and dispatching logic; AppType subclasses from typing_extensions import Self -__all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumRegistry'] +__all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumLookup', 'EnumRegistry'] @final class NoDefaultType: - """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for :meth:`EnumRegistry.get`. + """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 @@ -81,7 +121,7 @@ class NoDefaultType: on #859 is explicit that ``NULL`` (``SCREAMING_CASE``) and ``NoValue`` (``CapWords``) disagree, and "mainly depends on how we need it." The need here is continuity: ``NO_DEFAULT`` is already the name on ``main`` -- - referenced in :meth:`EnumRegistry.get`'s signature, its docstring, and + referenced in :meth:`EnumLookup.get`'s signature, its docstring, and both comparison sites -- and this change is to *what the sentinel is*, not to *what it is called*, so it keeps that name rather than being renamed to match either precedent's instance casing for its own sake. @@ -94,7 +134,7 @@ class NoDefaultType: caller writing ``registry.get(key, default=NoDefaultType())`` -- perhaps not realising :data:`NO_DEFAULT` already exists -- and getting back a *second*, non-identical sentinel that silently fails ``is NO_DEFAULT`` - inside :meth:`EnumRegistry.get`, so their call is treated as supplying a + inside :meth:`EnumLookup.get`, so their call is treated as supplying a real (if useless) default instead of the *no default* they meant. With the guard, :class:`NoDefaultType() ` always returns the one canonical :data:`NO_DEFAULT`, so that mistake self-corrects. @@ -130,8 +170,8 @@ class NoDefaultType: :func:`copy.deepcopy` or :mod:`pickle` call can walk into and reconstruct. :data:`NO_DEFAULT` is left unhandled here not because the gap cannot occur in principle, but because nothing in this package ever pickles it at - protocol 0: it is reachable -- from :meth:`EnumRegistry.get`'s own bound - parameter default (``EnumRegistry.get.__defaults__[0]``, or any + protocol 0: it is reachable -- from :meth:`EnumLookup.get`'s own bound + parameter default (``EnumLookup.get.__defaults__[0]``, or any subclass's, e.g. ``Hardware.get.__func__.__defaults__[0]``) and from ``inspect.signature(Hardware.get).parameters['default'].default`` -- but neither is a field any object here gets pickled *as*, and both still @@ -141,11 +181,11 @@ class NoDefaultType: A caveat, not a defect, but a real and *worse* one than the marker it replaces: :func:`importlib.reload` on this module does not merely leave - the class stale, it breaks :meth:`EnumRegistry.get`'s own no-default + the class stale, it breaks :meth:`EnumLookup.get`'s own no-default contract for every subclass whose ``get`` was already resolved before the reload. Reload re-executes both ``class NoDefaultType:`` and ``NO_DEFAULT = NoDefaultType()`` below, so the *module global* - :meth:`EnumRegistry.get`'s body compares against becomes a fresh, distinct + :meth:`EnumLookup.get`'s body compares against becomes a fresh, distinct object. But each subclass's own ``get`` inherited its **bound parameter default** -- ``default: 'Any' = NO_DEFAULT`` -- at function-definition time, before the reload, and a bound default is frozen then, not looked @@ -183,7 +223,7 @@ class NoDefaultType: *not* define ``__bool__``. Both of those model an *absent* value, so reading falsy in a boolean context is the point. :data:`NO_DEFAULT` models something different: a marker meaning *no default was supplied*, checked - exclusively by ``is`` at :meth:`EnumRegistry.get`'s two comparison sites -- + exclusively by ``is`` at :meth:`EnumLookup.get`'s two comparison sites -- nothing here ever evaluates it for truthiness. Giving it ``__bool__ -> False`` for symmetry with the other two would invite exactly the conflation this sentinel exists to rule out: code that writes ``if not @@ -225,23 +265,38 @@ def __repr__(self) -> 'str': NO_DEFAULT = NoDefaultType() -class EnumRegistry: - """Registry protocol shared by every constant enumeration under - :mod:`pcapkit.const`. +class EnumLookup: + """Bare lookup protocol, shared by open registries and closed sets alike. + + Carries :meth:`get`, :meth:`get_all` and the :meth:`_validate_value` guard -- + everything an enumeration needs in order to be *read* by name or by value, + and nothing that could grow it. :class:`EnumRegistry` adds the mutating half + on top; a closed enumeration inherits this one directly and so is never + handed a ``register`` it would have to refuse. This is a plain mix-in rather than an :class:`~aenum.Enum` subclass, because an enumeration that already has members cannot be subclassed. Mixed in - *before* the member type -- ``class Foo(EnumRegistry, IntFlag)`` -- it - contributes methods only, so :mod:`aenum` still resolves the member data type - from the enumeration base: ``int`` for :class:`~aenum.IntEnum` and - :class:`~aenum.IntFlag`, ``str`` for :class:`~aenum.StrEnum`. That is what - lets one base serve all three, where a generated template fragment would - have needed a separate rendering per member type. + *before* the member type -- ``class Foo(EnumRegistry, IntFlag)``, or + ``class Bar(EnumLookup, IntEnum)`` -- it contributes methods only, so + :mod:`aenum` still resolves the member data type from the enumeration base: + ``int`` for :class:`~aenum.IntEnum` and :class:`~aenum.IntFlag`, ``str`` for + :class:`~aenum.StrEnum`. That is what lets one base serve all three, where a + generated template fragment would have needed a separate rendering per + member type. + + Because both tiers are plain classes, inserting this one *above* + :class:`EnumRegistry` leaves the member data type exactly where it was: + ``class Foo(EnumRegistry, IntFlag)`` resolves as ``Foo -> EnumRegistry -> + EnumLookup -> IntFlag -> int -> ...``, so ``_member_type_`` still comes from + the enumeration base and not from anything in this module. Had this tier + subclassed :class:`~aenum.Enum` in order to "be an enum", it would have + become the member type itself and broken all three shapes at once. The methods deliberately touch only ``_member_map_``, ``_member_names_`` and ``_value2member_map_``, which both :mod:`enum` and :mod:`aenum` maintain, so - nothing here depends on :mod:`aenum` internals beyond - :func:`~aenum.extend_enum` itself. + nothing on this tier depends on :mod:`aenum` internals at all -- the one + :func:`~aenum.extend_enum` call in this module belongs to + :class:`EnumRegistry`, which is the tier that mutates. """ @@ -260,6 +315,82 @@ class EnumRegistry: # subclass picks, so there is nothing more precise to say here anyway. _member_type_: 'Any' + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Hook: reject ``value`` if this enumeration's contract does not allow it. + + The owner's requirement on GitHub issue #877, verbatim: *"there must be + some sort of range validation logic for the inherited classes to hook + in."* This is that hook, and it is what the bare tier carries **instead** + of ``register``: what values are *legal* is something every enumeration + has an opinion on, whereas who may *add* one is only an open registry's + concern. + + The base implementation accepts everything, because a base cannot know + any subclass's range. Overriding it is how a subclass states one -- the + shape ``_missing_`` spells by hand across the generated registries + today:: + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + if not (isinstance(value, int) and 0 <= value <= 0xFF): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + An override **raises or returns**; it must never *normalise*. The return + type is :obj:`None` deliberately rather than the validated value, so that + this hook cannot become a converter: a subclass that returned a changed + value here would silently alter what a lookup resolves to, which is + exactly the case-folding the owner's ruling on GitHub issue #877 rules + out -- *"enum should honour and keep their original writings as in the + registrars."* Case handling belongs in a deliberate ``get`` override with + an RFC behind it, not in a validation hook. + + Raise from :mod:`pcapkit.utilities.exceptions`, per the same issue's + ruling that in-library code raises in-library exceptions -- + :exc:`~pcapkit.utilities.exceptions.EnumValueError` is the fitting one + and is already what the closed enumerations in + :mod:`pcapkit.protocols.internet.mh` raise. Note what that buys on the + :meth:`get` path: :exc:`~pcapkit.utilities.exceptions.EnumValueError` + subclasses :exc:`ValueError`, so a rejection here is caught by + :meth:`get`'s own ``except ValueError`` and falls back to ``default`` + just as any other unresolvable value does. An override raising something + outside that hierarchy would instead propagate past ``default``, which is + a real difference in behaviour rather than a stylistic preference. + + Called from exactly two places, and the omissions are deliberate: + + * :meth:`get`, immediately before ``cls(key)`` -- the one point at which + a lookup can reach a subclass's ``_missing_`` and mint. The ``str``-key + path does **not** call it, because that path never calls ``cls(key)``: + it resolves against the already-populated lookup tables only, where + every value present is legal by construction, so there is nothing left + to validate. + * :meth:`EnumRegistry.register`, before minting -- the one point at which + a caller can introduce a value no member carries yet. + + :meth:`EnumRegistry.register_alias` does not call it, and does not need + to: it refuses any ``value`` that is not already registered, so the value + it aliases has necessarily passed validation already. + :meth:`EnumRegistry._unregistered_member` does not call it either, + because its callers are the subclasses' own ``_missing_`` bodies, which + already range-check before delegating here -- validating again would + double the check without being able to disagree with it. + + Deliberately carries no ``Raises:`` clause, because this implementation + raises nothing at all -- an override is what raises, and documenting an + exception here that this body cannot produce is exactly the phantom + :class:`tests.test_docstring_contract.DocstringRaisesTests` rejects. + An override adds its own clause naming what *it* rejects. + + Args: + value: Candidate value to check. + + Returns: + Nothing. A value this enumeration allows is reported by returning + normally; a value it does not is reported by raising. + + """ + @classmethod def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': """Resolve ``key`` to the canonical member. @@ -301,16 +432,20 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': unrecognised value, routing a failed *name* lookup through the constructor would let a mere ``get()`` call mint a permanent member where it previously just raised. Defensive rather than observed: of - the 124 classes that mix in this base, the ``str``-valued ones + the 125 classes that reach this method, the ``str``-valued ones (:class:`~pcapkit.const.ftp.command.Command`, :class:`~pcapkit.const. ftp.command.FEATCode`, :class:`~pcapkit.const.http.method.Method`, - :class:`~pcapkit.const.pcapng.option_type.OptionType`, and -- - completing the count as of GitHub issue #860's own PR 2 -- + :class:`~pcapkit.const.pcapng.option_type.OptionType`, :class:`~pcapkit.const.reg.apptype.apptype.AppType` and its four transport subclasses :class:`~pcapkit.const.reg.apptype.tcp.TCP`, :class:`~pcapkit.const.reg.apptype.udp.UDP`, :class:`~pcapkit.const.reg.apptype.sctp.SCTP` and - :class:`~pcapkit.const.reg.apptype.dccp.DCCP`) no longer mint + :class:`~pcapkit.const.reg.apptype.dccp.DCCP` -- completing that set as + of GitHub issue #860's own PR 2 -- and, newest of them, + :class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel`, which GitHub + issue #877's own thread reclassified from a hand-written helper to a + generated registry once RFC 9850 ยง4.2 turned its member list into a + live IANA registry) no longer mint on any path, so no live witness exists in this tree today. The one registry that still mints directly via :func:`~aenum.extend_enum`, :class:`~pcapkit.const.mh.cga_type.CGAType`, is @@ -345,6 +480,18 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': which reopens the exact minting hazard the paragraph above exists to avoid -- so the asymmetry is deliberate, not an oversight. + The non-``str`` path calls :meth:`_validate_value` immediately before + ``cls(key)``, which is the only point at which this method can reach a + subclass's ``_missing_``, so a subclass that declares a range gets it + checked before the constructor rather than after. The base hook accepts + everything, so this changes nothing for a subclass that does not override + it. A rejection raised as + :exc:`~pcapkit.utilities.exceptions.EnumValueError` -- or any other + :exc:`ValueError` subclass -- is caught by the same ``except`` that + catches an ordinary failed construction, and so falls back to ``default`` + on the same terms; the ``str`` path does not call the hook, for the reason + given on :meth:`_validate_value` itself. + Args: key: Name or value to look up. default: An already-registered value to fall back to when @@ -360,7 +507,10 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': Raises: ValueError: If a value does not resolve and there is no usable - default. + default -- including a value a subclass's + :meth:`_validate_value` rejects, since + :exc:`~pcapkit.utilities.exceptions.EnumValueError` is a + :exc:`ValueError`. KeyError: If a name does not resolve and there is no usable default. @@ -375,6 +525,7 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': raise return cls._value2member_map_[default] try: + cls._validate_value(key) return cls(key) # type: ignore[call-arg] except ValueError: if default is NO_DEFAULT or default not in cls._value2member_map_: @@ -412,6 +563,29 @@ def get_all(cls, key: 'Any') -> 'tuple[Self, ...]': and member.value == canonical.value # type: ignore[attr-defined] )) + +class EnumRegistry(EnumLookup): + """Registry protocol shared by every constant enumeration under + :mod:`pcapkit.const`. + + :class:`EnumLookup` above carries the read half -- :meth:`~EnumLookup.get`, + :meth:`~EnumLookup.get_all` and :meth:`~EnumLookup._validate_value`, all + inherited here unchanged. What this tier adds is the half that makes a + registry *open*: :meth:`register`, :meth:`register_alias`, + :meth:`register_aliases`, :meth:`_extend` and :meth:`_unregistered_member`. + + An enumeration inherits from *here* when it may grow at runtime, and from + :class:`EnumLookup` directly when it may not. The owner's ruling on GitHub + issue #877 is what draws that line, verbatim: *"My initial thought was to + make them immutable - unless RFC/IANA says otherwise."* + + Mixed in ahead of the enum base exactly as before -- ``class + Foo(EnumRegistry, IntFlag)`` -- and gaining :class:`EnumLookup` as a parent + does not disturb that: both tiers are plain classes, so ``_member_type_`` + still resolves past them to the enumeration base. + + """ + @classmethod def register(cls, value: 'Any', name: 'str') -> 'Self': """Mint a new member on this registry at runtime, under ``name``. @@ -436,6 +610,14 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': :meth:`_unregistered_member` absent from that table, so a successful call proves nothing about whether a member already exists. + Routes through :meth:`~EnumLookup._validate_value` before minting, so a + subclass that declares a range gets it enforced on the caller-named path + too and not only on the lookup one. The duplicate check runs *first*: a + ``value`` that already has a member is legal by construction, so the + actionable "use ``register_alias()`` instead" message is the better answer + for it than a range complaint would be, and validation is left to guard + only the genuinely new value that is about to be minted. + Args: value: Value of the new member. name: Name of the new member. Required rather than derived, which @@ -447,6 +629,10 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': Raises: ValueError: If ``value`` already has a member -- use :meth:`register_alias` to add a further name for it instead. + EnumValueError: If a subclass's + :meth:`~EnumLookup._validate_value` rejects ``value``. The base + implementation of that hook accepts everything, so this cannot + arise on a registry that does not override it. ValueError: If ``name`` is already taken. :mod:`aenum` reports that as :exc:`TypeError`; it is translated so that the ways one call can fail are one exception type. @@ -457,6 +643,7 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': raise ValueError(f'{value!r} is already registered on {cls.__name__} as ' f'{existing.name!r}; use {cls.__name__}.register_alias() ' f'to add a further name for it') + cls._validate_value(value) return cls._extend(value, name) @classmethod diff --git a/tests/corekit/test_enum_lookup_base_unit.py b/tests/corekit/test_enum_lookup_base_unit.py new file mode 100644 index 0000000000..cc9a62eb06 --- /dev/null +++ b/tests/corekit/test_enum_lookup_base_unit.py @@ -0,0 +1,542 @@ +# -*- coding: utf-8 -*- +"""Phase 1 of GitHub issue #877: the bare lookup base under :class:`EnumRegistry`. + +The owner's ruling, verbatim: *"they may subclass a bare base enum from +pcapkit.corekit.enum - where EnumRegistry subclasses it for using in the other +mutable ones."* :class:`~pcapkit.corekit.enum.EnumLookup` is that base, and it +carries :meth:`~pcapkit.corekit.enum.EnumLookup.get`, +:meth:`~pcapkit.corekit.enum.EnumLookup.get_all` and the overridable +:meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` hook -- not ``register``, +because the owner's own second thought settled that: *"if it carries ``register``, +then why not ``register_alias``. We might be creating a bad ruling."* + +What this module pins, and why each part is worth pinning: + +* the split is where it was ruled to be -- three methods on the base, the five + mutating ones still on :class:`~pcapkit.corekit.enum.EnumRegistry`, asserted as + an **exact** partition rather than as mere presence, so that a later change + cannot quietly migrate one across (:class:`TwoTierSplitTests`); +* the base works as a base, for an :class:`int`-valued and a :class:`str`-valued + enumeration alike, without either being a registry (:class:`BareLookupTests`); +* the extra tier does not become the member data type, which is the one way a + mix-in like this breaks all three enumeration shapes at once + (:class:`MemberTypeTests`); +* :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` accepts everything by + default, is honoured on both the paths that call it, and is *not* called on the + ``str``-key path -- that last one is a documented omission, so it is asserted + rather than left to be rediscovered (:class:`ValidateValueTests`); +* the declared-but-unassigned asymmetry :meth:`get`'s own docstring describes + survives on the new base (:class:`UnassignedAsymmetryTests`); +* members still pickle, on every protocol, for all three shapes + (:class:`PickleTests`) -- flagged unverified while the change was being + designed, since inserting a class into the MRO is exactly the kind of thing + that can disturb :meth:`~object.__reduce_ex__`. + +On the tree before this change every test below fails at **import**:: + + ImportError: cannot import name 'EnumLookup' from 'pcapkit.corekit.enum' + +That is not an artefact of how the module is written: the classes these tests +exercise inherit from the base being introduced, so there is nothing to exercise +until it exists. Two of the tests fail pre-change for a second, independent +reason as well -- :meth:`TwoTierSplitTests.test_the_split_is_an_exact_partition` +and :meth:`TwoTierSplitTests.test_enum_registry_subclasses_the_lookup_base` +both contradict the single-tier shape even when rewritten against +:class:`~pcapkit.corekit.enum.EnumRegistry` alone, because ``get`` and +``get_all`` live in ``EnumRegistry.__dict__`` there. + +""" +from __future__ import annotations + +import pickle +import unittest +from typing import TYPE_CHECKING + +from aenum import IntEnum, IntFlag, StrEnum + +from pcapkit.corekit.enum import NO_DEFAULT, EnumLookup, EnumRegistry +from pcapkit.utilities.exceptions import EnumValueError + +if TYPE_CHECKING: + from typing import Any + +#: The mutating half, which must stay on :class:`~pcapkit.corekit.enum.EnumRegistry`. +MUTATING = ('register', 'register_alias', 'register_aliases', '_extend', + '_unregistered_member') + +#: The lookup half, which must move to :class:`~pcapkit.corekit.enum.EnumLookup`. +LOOKUP = ('get', 'get_all', '_validate_value') + + +class _Int(EnumLookup, IntEnum): + """A closed ``int``-valued set on the bare base -- no registry anywhere.""" + + one = 1 + two = 2 + + +class _Str(EnumLookup, StrEnum): + """A closed ``str``-valued set on the bare base. + + ``angled`` deliberately carries a value that is not also a member name, which + is what makes the value-fallback path in + :meth:`~pcapkit.corekit.enum.EnumLookup.get` observable -- the same shape as + :class:`~pcapkit.const.ftp.command.FEATCode`'s own ``base = ''``. + + """ + + plain = 'plain' + angled = '' + + +class _Flag(EnumLookup, IntFlag): + """A closed flag set on the bare base.""" + + first = 1 + second = 2 + + +class _Ranged(EnumLookup, IntEnum): + """Hooks a range in, the way the generated registries spell it in ``_missing_``.""" + + low = 1 + high = 8 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Reject anything outside ``0 <= value <= 8``.""" + if not (isinstance(value, int) and 0 <= value <= 8): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + +#: 'list[Any]': Values :class:`_Counting`'s hook was handed, in order. Module +#: level rather than a class attribute because any non-descriptor assigned in an +#: enumeration body becomes a *member* -- ``seen = []`` inside the class below +#: raises ``TypeError: values must be str``, which is how this was found. +COUNTING_SEEN = [] # type: list[Any] + + +class _Counting(EnumLookup, StrEnum): + """Records every :meth:`_validate_value` call, to pin the ``str``-path omission.""" + + plain = 'plain' + angled = '' + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Record ``value`` and accept it.""" + COUNTING_SEEN.append(value) + + +class _PickleInt(EnumRegistry, IntEnum): + """Registry shape ``int``, for the pickle round-trip.""" + + solo = 7 + + +class _PickleStr(EnumRegistry, StrEnum): + """Registry shape ``str``, for the pickle round-trip.""" + + solo = 'solo' + + +class _PickleFlag(EnumRegistry, IntFlag): + """Registry shape flag, for the pickle round-trip.""" + + solo = 4 + + +class TwoTierSplitTests(unittest.TestCase): + """The partition between the two tiers, asserted exactly.""" + + def test_enum_registry_subclasses_the_lookup_base(self) -> 'None': + """:class:`EnumRegistry` is a child of :class:`EnumLookup`, not a sibling. + + A sibling could not share ``get``; a *subclass* is the shape the ruling + names, and it is also what keeps ``_member_type_`` alone -- both tiers are + plain classes, so neither becomes the member data type. + + """ + self.assertTrue(issubclass(EnumRegistry, EnumLookup)) + self.assertIsNot(EnumRegistry, EnumLookup) + self.assertEqual(EnumRegistry.__mro__, (EnumRegistry, EnumLookup, object)) + + def test_the_split_is_an_exact_partition(self) -> 'None': + """Each method is defined on exactly one tier, and on the right one. + + Asserted against ``__dict__`` rather than with :func:`hasattr`, which + cannot tell "defined here" from "inherited": the mutating five are + reachable from :class:`EnumLookup`'s subclasses either way, so presence + alone would pass on the single-tier tree this change replaces. + + """ + for name in LOOKUP: + with self.subTest(method=name, tier='EnumLookup'): + self.assertIn(name, EnumLookup.__dict__) + self.assertNotIn(name, EnumRegistry.__dict__) + for name in MUTATING: + with self.subTest(method=name, tier='EnumRegistry'): + self.assertIn(name, EnumRegistry.__dict__) + self.assertNotIn(name, EnumLookup.__dict__) + + def test_the_bare_base_offers_no_way_to_mutate(self) -> 'None': + """A closed set on the base cannot be grown, which is the whole point.""" + for name in MUTATING: + with self.subTest(method=name): + self.assertFalse(hasattr(_Int, name)) + self.assertFalse(hasattr(EnumLookup, name)) + + def test_registries_still_reach_every_mutating_method(self) -> 'None': + """And the registry tier keeps all five, inherited or not.""" + for name in MUTATING: + with self.subTest(method=name): + self.assertTrue(callable(getattr(_PickleInt, name))) + + def test_lookup_methods_resolve_to_the_base_on_a_registry(self) -> 'None': + """A registry's ``get`` is now the base's, not a copy of it.""" + for name in ('get', 'get_all'): + with self.subTest(method=name): + self.assertIs(getattr(_PickleInt, name).__func__, + getattr(EnumLookup, name).__func__) + + +class BareLookupTests(unittest.TestCase): + """:meth:`get` and :meth:`get_all` on closed sets that are not registries.""" + + def test_int_lookup_by_name_and_by_value(self) -> 'None': + """Both keys resolve to the one canonical member.""" + self.assertIs(_Int.get('one'), _Int.one) + self.assertIs(_Int.get(1), _Int.one) + self.assertIs(_Int.get(2), _Int.two) + + def test_str_lookup_by_name_and_by_value(self) -> 'None': + """Including a value that is not also a member name. + + ``_Str.get('')`` is the measurement + :file:`docs/source/conventions.rst` records on + :class:`~pcapkit.const.ftp.command.FEATCode`, made here on a class that + cannot be overriding ``get``, since it defines none. + + """ + self.assertIs(_Str.get('plain'), _Str.plain) + self.assertNotIn('', _Str._member_map_) + self.assertIs(_Str.get(''), _Str.angled) + + def test_get_is_case_sensitive(self) -> 'None': + """The ruled default: *"otherwise, we should treat them case sensitive."* + + Case-insensitivity is a per-class override needing an RFC behind it, so + the base must not fold case itself. Auditing the existing overrides + against their specifications is GitHub issue #903. + + """ + with self.assertRaises(KeyError): + _Str.get('PLAIN') + with self.assertRaises(KeyError): + _Int.get('ONE') + + def test_get_all_returns_the_canonical_member_alone(self) -> 'None': + """One key, one member, for a set with no duplicate values.""" + self.assertEqual(_Int.get_all('one'), (_Int.one,)) + self.assertEqual(_Int.get_all(1), (_Int.one,)) + self.assertEqual(_Str.get_all('plain'), (_Str.plain,)) + + def test_default_resolves_only_through_registered_values(self) -> 'None': + """A usable ``default`` is returned; an unusable one re-raises.""" + self.assertIs(_Int.get(99, default=1), _Int.one) + self.assertIs(_Str.get('nope', default='plain'), _Str.plain) + with self.assertRaises(ValueError): + _Int.get(99, default=98) + + def test_omitted_default_propagates_the_lookup_error(self) -> 'None': + """:data:`NO_DEFAULT` means *no default*, not a default of its own.""" + with self.assertRaises(ValueError): + _Int.get(99) + with self.assertRaises(ValueError): + _Int.get(99, default=NO_DEFAULT) + with self.assertRaises(KeyError): + _Str.get('nope') + + def test_lookups_do_not_grow_a_closed_set(self) -> 'None': + """No path through the base registers anything. + + ``Flag`` is excluded deliberately and gets its own test below: its + composite caching is the owner's one ruled exception, not a leak here. + + """ + for cls in (_Int, _Str): + with self.subTest(cls=cls.__name__): + before = (list(cls._member_names_), dict(cls.__members__), + dict(cls._value2member_map_)) + for probe in ('nope', 99, -1): + try: + cls.get(probe) + except (KeyError, ValueError): + pass + self.assertEqual(list(cls._member_names_), before[0]) + self.assertEqual(dict(cls.__members__), before[1]) + self.assertEqual(dict(cls._value2member_map_), before[2]) + + def test_flag_composites_remain_the_ruled_exception(self) -> 'None': + """A ``Flag`` still caches composites, and that is sanctioned. + + The owner's ruling on this issue, verbatim: *"Flag subclasses is the one + only exception where we're expecting values to grow and fill due to + combinations."* So ``_Flag.get(99)`` composing ``first|second|96`` into + ``_value2member_map_`` is :class:`~aenum.Flag`'s own machinery behaving as + expected, and the base does not -- and must not -- suppress it. + + The distinction worth pinning is *which* table moves: no real member is + added, so ``_member_names_`` and ``__members__`` are untouched while the + value cache grows. That is what separates a composite from a mint. + + """ + names_before = list(_Flag._member_names_) + members_before = dict(_Flag.__members__) + + composite = _Flag.get(3) + self.assertEqual(composite, _Flag.first | _Flag.second) + + self.assertEqual(list(_Flag._member_names_), names_before) + self.assertEqual(dict(_Flag.__members__), members_before) + self.assertIn(3, _Flag._value2member_map_) + + +class MemberTypeTests(unittest.TestCase): + """The extra tier must not become the member data type.""" + + def test_member_type_survives_the_extra_tier(self) -> 'None': + """``int`` for ``IntEnum``/``IntFlag``, ``str`` for ``StrEnum``.""" + self.assertIs(_Int._member_type_, int) + self.assertIs(_Flag._member_type_, int) + self.assertIs(_Str._member_type_, str) + + def test_member_type_survives_it_under_a_registry_too(self) -> 'None': + """The case that would regress: two plain tiers ahead of the enum base.""" + self.assertIs(_PickleInt._member_type_, int) + self.assertIs(_PickleFlag._member_type_, int) + self.assertIs(_PickleStr._member_type_, str) + + def test_members_still_behave_as_their_data_type(self) -> 'None': + """Not merely annotated as ``int``/``str`` -- actually usable as one.""" + self.assertEqual(int(_Int.one), 1) + self.assertEqual(str(_Str.plain), 'plain') + self.assertEqual(int(_PickleFlag.solo), 4) + self.assertEqual(_Int.one + 1, 2) + + def test_neither_tier_is_an_enumeration(self) -> 'None': + """Both are plain mix-ins, which is what lets one base serve all shapes.""" + for tier in (EnumLookup, EnumRegistry): + with self.subTest(tier=tier.__name__): + self.assertFalse(hasattr(tier, '__members__')) + + +class ValidateValueTests(unittest.TestCase): + """The hook the owner asked for: *"some sort of range validation logic."*""" + + def test_the_default_hook_accepts_everything(self) -> 'None': + """A base cannot know any subclass's range, so it forbids nothing.""" + for probe in (0, -1, 2 ** 64, 'anything', None, object()): + with self.subTest(probe=type(probe).__name__): + self.assertIsNone(EnumLookup._validate_value(probe)) + self.assertIsNone(_Int._validate_value(probe)) + + def test_an_override_is_honoured_on_the_value_path(self) -> 'None': + """An out-of-range value is refused before the constructor sees it.""" + self.assertIs(_Ranged.get(1), _Ranged.low) + with self.assertRaises(EnumValueError): + _Ranged.get(99) + with self.assertRaises(EnumValueError): + _Ranged.get(-1) + + def test_a_rejection_still_honours_default(self) -> 'None': + """Because :exc:`EnumValueError` is a :exc:`ValueError`. + + Documented on the hook, and worth an assertion: an override raising + outside that hierarchy would propagate past ``default`` instead, which is + a behavioural difference rather than a stylistic one. + + """ + self.assertTrue(issubclass(EnumValueError, ValueError)) + self.assertIs(_Ranged.get(99, default=1), _Ranged.low) + + def test_an_in_range_but_unassigned_value_still_fails_to_resolve(self) -> 'None': + """The hook grants legality, not membership. + + ``4`` is inside ``_Ranged``'s declared range and carries no member, so it + passes validation and is then refused by the enumeration itself -- the + hook is a guard in front of the lookup, not a substitute for it. + + """ + self.assertIsNone(_Ranged._validate_value(4)) + with self.assertRaises(ValueError): + _Ranged.get(4) + + def test_the_str_key_path_does_not_call_the_hook(self) -> 'None': + """A documented omission, so pinned rather than left to be rediscovered. + + That path resolves against the already-populated lookup tables only, where + every value present is legal by construction. + + """ + COUNTING_SEEN.clear() + self.assertIs(_Counting.get('plain'), _Counting.plain) + self.assertIs(_Counting.get(''), _Counting.angled) + with self.assertRaises(KeyError): + _Counting.get('nope') + self.assertEqual(COUNTING_SEEN, []) + + # Not vacuous: the same hook on the same class *is* reached from the + # non-str path, so the empty list above is the omission and not a hook + # that never fires. + with self.assertRaises(ValueError): + _Counting.get(99) + self.assertEqual(COUNTING_SEEN, [99]) + + def test_register_routes_through_the_hook(self) -> 'None': + """The mutating path validates too, and nothing is minted when it refuses.""" + class _Registry(EnumRegistry, IntEnum): + low = 1 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + if not (isinstance(value, int) and 0 <= value <= 8): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + before = list(_Registry._member_names_) + with self.assertRaises(EnumValueError): + _Registry.register(99, 'rejected') + self.assertEqual(list(_Registry._member_names_), before) + self.assertNotIn('rejected', _Registry.__members__) + + minted = _Registry.register(5, 'accepted') + self.assertEqual(minted.value, 5) + self.assertEqual(minted.name, 'accepted') + self.assertIn('accepted', _Registry.__members__) + + def test_the_duplicate_check_runs_before_validation(self) -> 'None': + """So the actionable message wins over a range complaint. + + ``declared`` sits outside the class's own validated range, which is how + the ordering becomes observable at all: validate-first would answer an + already-taken value with :exc:`EnumValueError`, losing the *"use + register_alias()"* hint that is the useful answer. + + """ + class _Narrow(EnumRegistry, IntEnum): + declared = 100 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + if not (isinstance(value, int) and 0 <= value <= 8): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + with self.assertRaises(ValueError) as caught: + _Narrow.register(100, 'again') + self.assertNotIsInstance(caught.exception, EnumValueError) + self.assertIn('register_alias', str(caught.exception)) + + def test_register_alias_does_not_need_the_hook(self) -> 'None': + """An aliased value is already registered, so it already passed.""" + class _Aliased(EnumRegistry, IntEnum): + declared = 100 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + raise EnumValueError('this hook refuses everything') + + aliased = _Aliased.register_alias(100, 'nickname') + self.assertIs(aliased, _Aliased.declared) + self.assertIn('nickname', _Aliased.__members__) + + +class UnassignedAsymmetryTests(unittest.TestCase): + """The declared-but-unassigned ``str`` gap, unchanged on the new base. + + :meth:`~pcapkit.corekit.enum.EnumLookup.get`'s own docstring calls this + deliberate: closing it would mean calling ``cls(key)`` for a ``str`` value, + which reopens the minting hazard the ``str`` path exists to avoid. Pinned + here so that a later "fix" has to argue with a test. + + """ + + def test_a_str_value_reached_only_through_missing_does_not_resolve(self) -> 'None': + """``cls(value)`` yields it; ``get(value)`` does not.""" + class _Unassigned(EnumRegistry, StrEnum): + plain = 'plain' + + @classmethod + def _missing_(cls, value: 'Any') -> 'Any': + return cls._unregistered_member(value, 'unassigned') + + member = _Unassigned('ZZ-NOT-REAL') + self.assertEqual(member.value, 'ZZ-NOT-REAL') + self.assertEqual(member.name, 'unassigned') + + with self.assertRaises(KeyError): + _Unassigned.get('ZZ-NOT-REAL') + + def test_neither_path_grows_the_lookup_tables(self) -> 'None': + """Which is why the value stays invisible to ``get`` in the first place.""" + class _Untouched(EnumRegistry, StrEnum): + plain = 'plain' + + @classmethod + def _missing_(cls, value: 'Any') -> 'Any': + return cls._unregistered_member(value, 'unassigned') + + before = (len(_Untouched._member_names_), len(_Untouched._value2member_map_), + len(_Untouched._member_map_)) + _Untouched('ZZ-NOT-REAL') + try: + _Untouched.get('ZZ-ALSO-NOT-REAL') + except KeyError: + pass + after = (len(_Untouched._member_names_), len(_Untouched._value2member_map_), + len(_Untouched._member_map_)) + self.assertEqual(before, after) + + +class PickleTests(unittest.TestCase): + """Members survive the round trip on every protocol, for all three shapes. + + Inserting a class into the MRO is exactly the sort of change that can disturb + :meth:`~object.__reduce_ex__`, and this was flagged unverified while the + change was being designed. Each case also asserts that the class really does + reach the new base, so the test cannot pass vacuously on the single-tier tree + this change replaces. + + """ + + def assertRoundTrips(self, member: 'Any') -> 'None': + """Pickle and unpickle ``member`` on every protocol.""" + self.assertIn(EnumLookup, type(member).__mro__) + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + with self.subTest(protocol=protocol): + back = pickle.loads(pickle.dumps(member, protocol=protocol)) + self.assertIs(back, member) + self.assertEqual(back.name, member.name) + self.assertEqual(back.value, member.value) + self.assertIs(type(back), type(member)) + + def test_int_registry_member_round_trips(self) -> 'None': + """``EnumRegistry`` + ``IntEnum``.""" + self.assertRoundTrips(_PickleInt.solo) + + def test_str_registry_member_round_trips(self) -> 'None': + """``EnumRegistry`` + ``StrEnum``.""" + self.assertRoundTrips(_PickleStr.solo) + + def test_flag_registry_member_round_trips(self) -> 'None': + """``EnumRegistry`` + ``IntFlag``.""" + self.assertRoundTrips(_PickleFlag.solo) + + def test_bare_base_members_round_trip_too(self) -> 'None': + """And a closed set on the base alone, which is what phase 2 will create.""" + self.assertRoundTrips(_Int.one) + self.assertRoundTrips(_Str.plain) + self.assertRoundTrips(_Flag.first) + + +if __name__ == '__main__': + unittest.main()