Skip to content

object() sentinel sweep for #640: three sites left, each a judgement call rather than a mechanical pass #661

Description

@JarryShaw

Follow-up to the review comment on #640, which asked for a sweep: "do another sweep to see if we're
having other places in the library using an object() sentinel - update it to follow the actual
class/object convention."

The sweep is done. #640 fixed its own site (_MISSING = object() became _Absent, an instance of a
@final _AbsentType following NoValueType). What is left is three sites, and none of them is a
mechanical pass — each is a judgement call with a reason to leave it alone, so they are written up
here rather than changed unilaterally.

What the sweep covered

Over pcapkit/, for: = object(); type('...', (), {})(); module- and class-level names matching
MISSING/SENTINEL/UNSET/NOT_SET/NOT_FOUND/NO_VALUE/ABSENT/UNDEFINED/OMITTED; any
default= whose absence is signalled by an identity comparison; every is / is not against a
non-builtin name; and any class existing only to be a singleton marker.

type('...', (), {})() has zero hits package-wide. object() has exactly one hit left,
item 3 below. Nothing under pcapkit/const/ or pcapkit/vendor/ holds an identity sentinel at all.

The inventory

1. NoValue / NoValueType — pcapkit/corekit/fields/field.py:26-36

Already the convention: @final class, __bool__ returning False, one module-level instance.
Nothing to do — this is the thing the other sites are being compared against.

Worth recording how far it travels, because it is the reason #640 did not simply reuse it: it is
documented (docs/source/pcapkit/corekit/fields/field.rst:17-18), it is the declared default of 11
public field constructors, it is returned from SwitchField.pre_process/post_process
(pcapkit/corekit/fields/misc.py:444, :474), it is written into the packet context
(pcapkit/protocols/schema/schema.py:851), it is stored on a schema attribute
(pcapkit/protocols/schema/internet/hopopt.py:709, ipv6_opts.py:714) and translated back to None
before it reaches the data model (pcapkit/protocols/internet/hopopt.py:1053,
ipv6_opts.py:1056), and it is baked as the default of every generated Schema.__init__
(pcapkit/protocols/schema/schema.py:71). It means "no value was given", which is not the question
#640's site asks.

2. _missing / _Missing — pcapkit/corekit/multidict.py:78-86

class _Missing:
    def __repr__(self) -> 'str':
        return "no value"

    def __reduce__(self) -> 'str':
        return "_missing"


_missing = _Missing()

Guards "did the caller pass a default to pop(), or should a missing key raise?" — used at
multidict.py:373, :393, :592, :596. None is genuinely unavailable: pop(key, None) is the
canonical dict idiom for "give me None rather than raise", so if the marker were None that call
would raise MissingKeyError.

This already satisfies the convention in the sense the review comment asked for — it is an
instance of a purpose-built class, not a bare object(). What it lacks against NoValueType is
@final and __bool__.

Recommendation: leave it, or add only @final. Two things make it more than a style edit:

  • It is the declared default of two public methods, so it is reachable without touching a
    private name (inspect.signature(MultiDict.pop).parameters['default'].default). The @overload
    stubs above each def spell it ..., so the type-checker-visible signature hides it while the
    runtime one does not.
  • __reduce__ returning the bare string "_missing" is pickle-by-name, which only resolves
    while pcapkit.corekit.multidict._missing is importable under exactly that name. Cross-process
    escape is deliberately supported, and tests/corekit/test_multidict.py:34-35 asserts both
    repr(...) == 'no value' and __reduce__() == '_missing'. Adding __bool__ returning False
    would also be a live behaviour change rather than a cosmetic one, since this instance is reachable
    by callers.

3. _NOT_FOUND = object() — pcapkit/utilities/compat.py:73

The only bare object() left in the package. Guards "has this cached_property already computed
its value?"
, at compat.py:106, :107, :110, :111. Genuinely module-private: val is
unconditionally overwritten before return val, it is not a parameter default, and git grep
finds the name nowhere else in the repo — not in another module, not in tests/, not in docs/.
None would not work: a cached_property legitimately returning None would recompute forever.

Recommendation: leave it. Two reasons, both about what the file is rather than about the
sentinel:

  • It is a verbatim backport of CPython's own functools.cached_property, which uses
    _NOT_FOUND = object() for exactly this. Renaming it to a house-convention singleton diverges the
    copy from the upstream it mirrors, which is the thing that makes a backport auditable.
  • It sits inside if sys.version_info < (3, 8): (opened at compat.py:71, with
    from functools import cached_property in the else:), so it is unreachable on 3.8+ and
    therefore untested and unmeasurable. pyproject.toml:49 still declares
    requires-python = ">=3.6, <4", so it is not dead by declaration, only in practice.

If the preference is house convention over upstream fidelity, this is a two-line change and I am
happy to make it — it just should be a decision rather than a drive-by.

Adjacent, and out of scope by the letter of the sweep

default: 'int' = -1 on the get() of 113 generated const enums. Same "absent versus supplied"
question, in the same semantic slot, but compared with == rather than is, so it is not an
identity sentinel. Representative: pcapkit/const/arp/hardware.py:150, :165. It is generated,
from the {NAME} template at pcapkit/vendor/default.py:75-92, plus nine vendor modules carrying
their own copy of the same template (pcapkit/vendor/ftp/return_code.py:145,
http/status_code.py:78, ipv6/extension_header.py:55, mh/binding_ack_flag.py:54,
mh/binding_update_flag.py:53, mh/handover_ack_flag.py:54, mh/handover_initiate_flag.py:54,
tcp/flags.py:70) — so any change belongs in the templates, never in pcapkit/const/, since
make vendor strips a hand edit.

Two reasons it is not a cleanup: -1 collides with a real code in any registry that has one, and it
is dispatched on across module boundaries — pcapkit/protocols/link/l2tpv2.py:237 and
pcapkit/protocols/link/ospf.py:191 both say in a comment that they dispatch on "the -1 sentinel, as
ARP does". Changing it is a behaviour change to 113 public get() signatures. Two enums already use
None for the same slot instead (pcapkit/const/ftp/command.py:288,
pcapkit/const/http/method.py:166), so the inconsistency is real; it is just much bigger than this
issue. Worth its own issue if it is wanted.

Checked and excluded, so nobody re-treads them

multidict.py:72/:74 (linked-list node identity); infoclass.py:82/:89 (class-object identity);
foundation/reassembly/data/data.py:60-90 (Completion, a three-valued StrEnum with a real
__bool__, not an absent-marker); corekit/context.py:244 (ordinary container truthiness);
corekit/fields/field.py:168-171 (ContextVar defaulting to None, no sentinel object);
protocols/schema/schema.py:956 (__missing__, a defaultdict hook, the #555 fix); every = ...
in an @overload or if TYPE_CHECKING: stub; and foundation/extraction.py:860-861, where the
'null'/'none' strings are the normalised form and None is already the absent marker
(pcapkit/__main__.py:90-93 records that inversion deliberately).

Suggested disposition

  1. _NOT_FOUND — decide upstream-fidelity versus house convention. Two lines either way.
  2. _missing — @final only, if anything; not __bool__, which callers can observe.
  3. The -1 const default — separate issue if wanted, since it is 113 generated files and a
    cross-module behaviour change.

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

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions