Skip to content

The four register_* registrars promise RegistryError for a non-class input and raise TypeError instead #1021

Description

@JarryShaw

All four registrar docstrings on main carry a Raises: clause whose first branch the code cannot reach:

  • pcapkit/foundation/extraction.py:397 — "If dumper is not a class, or not a Dumper subclass."
  • :438 — the same for engine / EngineBase
  • :483 — reassembly / ReassemblyBase
  • :524 — traceflow / TraceFlowBase

Each guard is a bare if not issubclass(x, Base):. A non-class never reaches the raise — it is rejected first. Measured, with the raise site read off the traceback rather than inferred from the message:

register_dumper      -> TypeError: issubclass() arg 1 must be a class | extraction.py:400
register_engine      -> TypeError: issubclass() arg 1 must be a class | <frozen abc>:123
register_reassembly  -> TypeError: issubclass() arg 1 must be a class | <frozen abc>:123
register_traceflow   -> TypeError: issubclass() arg 1 must be a class | <frozen abc>:123
register_protocol    -> TypeError: issubclass() arg 1 must be a class | <frozen abc>:123

register_protocol is affected too, so this is five registrars rather than four — pcapkit/foundation/registry/protocols.py:216. Its own Raises: clause makes no "not a class" claim, so its prose is accurate; the clause above covers the other four.

Two mechanisms, not one, which is why the identical message is misleading. Dumper's metaclass is plain type, so the builtin issubclass raises in place at the guard line. EngineBase, ReassemblyBase, TraceFlowBase and ProtocolBase all have ABCMeta-derived metaclasses (EngineMeta, ReassemblyMeta, TraceFlowMeta, ProtocolMeta), so the builtin delegates to ABCMeta.__subclasscheck__ and the raise happens inside abc. CPython emits the same string either way.

Two ways to close it, and the choice is a real one:

  1. Prose follows code — drop the "is not a class" branch from the four clauses, and if the TypeError is worth documenting, document it as a TypeError.
  2. Code follows prose — add an explicit isinstance(x, type) check ahead of each issubclass so callers get the promised RegistryError. A behaviour change on an error path, so it wants a changelog entry, and it would apply to all five for consistency.

Option 2 is the better contract — a registry rejecting bad input should raise the registry's own exception rather than leaking one from abc — but it is not a docs fix, so I am not assuming it.

The four clauses were introduced by #1015; the underlying guards predate it. Related: #1016, #1020.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    docsPull requests that change documentation only (docs: subject prefix)fixPull requests that fix a defect (fix: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions