Skip to content

fix(corekit): make every sentinel a singleton and give NO_VALUE a repr (#1841, #1842) - #1876

Merged
JarryShaw merged 3 commits into
mainfrom
fix/1841-novalue-repr
Oct 17, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
fix/1841-novalue-repr

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

  • Add a private abstract base, _Sentinel, to pcapkit.corekit.sentinels. It
    carries __new__, __copy__, __deepcopy__, __reduce__ and __repr__.
    So calling the type, copy, deepcopy and pickle at every protocol give back
    the one object for NULL, NO_VALUE, NO_DEFAULT and ABSENT.
  • __reduce__ pickles a sentinel by its module-level name. _get_null stays
    so that pickles written before this change still load.
  • NO_VALUE reprs as <NO_VALUE>, not as an object address.
  • Update the sentinel convention page and the sentinels API page.
  • Add tests: singleton behaviour, pickles written by the previous module,
    a parsed HOPOPT MPL schema and a deep-copied field, and the repr.

Closes #1841
Closes #1842

Known limit, documented and pinned by a test: a pickle of NO_VALUE, NO_DEFAULT or ABSENT written before this change at protocol 0 or 1 still loads, but not as the singleton. Those pickles rebuild through copyreg._reconstructor, which calls object.__new__ directly, so the class cannot intervene. Protocol 2 and above, and every pickle written after this change, load as the singleton.

#1841, #1842)

- Add a private abstract base, `_Sentinel`, to `pcapkit.corekit.sentinels`. It
  carries `__new__`, `__copy__`, `__deepcopy__`, `__reduce__` and `__repr__`.
  So calling the type, copy, deepcopy and pickle at every protocol give back
  the one object for `NULL`, `NO_VALUE`, `NO_DEFAULT` and `ABSENT`.
- `__reduce__` pickles a sentinel by its module-level name. `_get_null` stays
  so that pickles written before this change still load.
- `NO_VALUE` reprs as `<NO_VALUE>`, not as an object address.
- Update the sentinel convention page and the sentinels API page.
- Add tests: singleton behaviour, pickles written by the previous module,
  a parsed HOPOPT MPL schema and a deep-copied field, and the repr.

Closes #1841
Closes #1842
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) design A design or decision issue: a pattern being decided rather than a defect or a request review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 2cf51800c. A Sonnet cross-review (author: Opus) confirmed:

  • Singletons. All four sentinels stay the same object through the constructor, copy.copy, copy.deepcopy and pickle at protocols 0–5. This holds on 3.14 and, run standalone, on 3.7, 3.12, 3.13 and 3.15. A parsed HOPOPT MPL schema keeps seed is NO_VALUE through all of them.
  • Unchanged behaviour. Only the NO_VALUE repr and the metaclass (ABCMeta) differ from main. Truthiness, the other reprs, __all__, hashing and equality are identical, nothing compares sentinel types by type(x) ==, and mypy is clean.
  • Tests. The new tests fail on main's sentinels.py, and the wider corekit, dumpkit, const and project runs pass.

One limit, stated for your call. My note recording the ruling added that pickles written before this change must load as the singleton. That holds at protocols 2–5, and for NULL at every protocol. An old protocol-0/1 pickle of NO_VALUE, NO_DEFAULT or ABSENT still loads, but as a second instance. Those pickles rebuild through copyreg._reconstructor, which calls object.__new__ with no hook the class can use (the reviewer tried a metaclass __call__, __setstate__ and __reduce_ex__). main behaves the same way, so this is not a regression. The PR documents and pins it, and fixing it would mean monkeypatching copyreg.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Maybe we can directly use Sentinel? Following the naming and ABC convention of the library.

Also, prefer making everything in a sentinel class immutable, such as _name. And their repr/etc. agree with common Python builtin singleton's behaviours.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Understood. The plan for this PR:

  • Public base. Rename _Sentinel to a public Sentinel ABC in pcapkit/corekit/sentinels.py, exported and documented with versionadded:: 1.5.0.
  • Immutability. Setting or deleting any attribute on a sentinel raises AttributeError, and so does reassigning _name or _instance on its type. That mirrors None.x = 1 and type(None).x = 1, which both fail.
  • Match the built-in singletons (None, Ellipsis, NotImplemented):
    • repr is the bare name, NO_VALUE rather than <NO_VALUE>;
    • calling the type returns the one instance;
    • copy, deepcopy and pickle keep identity;
    • a concrete sentinel type cannot be subclassed;
    • truthiness stays per sentinel, as it already is.

A worker will add this as a new commit. The review label resets when it lands.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 17, 2026
…uilt-in reprs (#1841, #1842)

- Rename the private `_Sentinel` base to `Sentinel`, export it, and document it;
  no alias is kept under the old name.
- Add `SentinelMeta`, an `abc.ABCMeta` subclass that makes each concrete
  sentinel's instance as its class statement runs, refuses attribute writes on
  every sentinel type with `TypeError`, and refuses subclassing or `register`
  on a concrete one, as `type(None)` does.
- Sentinel instances get empty `__slots__` and raise `AttributeError` on every
  attribute write or delete, as `None` does.
- A sentinel reprs and prints as its own name (`NO_VALUE`), as `None` does;
  `AbsentType` drops its own `__repr__`.
- Update the sentinels API page, the sentinel convention page, and the tests
  that pinned the old reprs; add parity tests against the built-in singletons.

Closes #1841
Closes #1842
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 751e85767. A Sonnet cross-review compared each sentinel with None, Ellipsis and NotImplemented.

  • Built-in parity. The sentinels behave the same in each of these:
    • repr, str and format give the bare name (NO_VALUE).
    • Calling the type returns the instance.
    • copy, deepcopy and pickle protocols 0–5 keep identity.
    • Setting or deleting an attribute raises AttributeError on an instance and TypeError on the type.
    • Subclassing a concrete type raises TypeError.
    • Each sentinel's truthiness is unchanged.
  • Nothing else breaks. ABC behaviour, typing.final, copyreg, pydoc/help and reload all work. Nothing in the repo sets attributes on a sentinel or subclasses one.
  • Release history. No bracketed repr reached a final release: v1.4.1.post2 had NULL as '(null)' and no NO_DEFAULT/ABSENT. So no new versionchanged is needed.
  • Old pickles. Pickles from the previous head load as the singletons. Pickles from main load correctly at protocols 2–5. At protocols 0 and 1, NO_VALUE, NO_DEFAULT and ABSENT pickled from main load as a second instance; the Sentinel docstring says so.
  • Tests bite. I ran two mutations myself. Allowing attribute assignment on the type fails 31 tests in test_sentinel_singleton_1842_unit.py, and neutering the subclass check fails 4.

Not blocking:

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.07% (unit tier, Python 3.15, 93c2a68ed, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19595 881 2618 867 92.11%
pcapkit/corekit 2502 70 792 36 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2914 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17406 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 512 2 158 4 99.10%
pcapkit/vendor 4429 2341 1018 156 43.62%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 751e85767. CI is green, but the non-blocking Python 3.8 (KTLO) leg (job 117306530034) fails one test this PR adds. main fails none on that leg, and the KTLO legs are meant to stay green.

tests/corekit/test_sentinel_exports_unit.py:551, in test_no_value_reprs_as_its_own_name, fails with KeyError: 'default':

parameter = inspect.signature(field).parameters['default']

On Python 3.8, inspect.signature of a Generic subclass such as StringField reports Generic.__new__'s (*args, **kwds) instead of __init__, so there is no default parameter. Python 3.9 changed that.

Fix: read the constructor's own signature, inspect.signature(field.__init__), which behaves the same on every version, and keep the assertion on the default parameter. The test is otherwise fine. The rest of the PR stays as reviewed.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 17, 2026
…ALUE repr test (#1841)

- `test_no_value_reprs_as_its_own_name` called `inspect.signature` on the field
  class. On Python 3.8 that reports `Generic.__new__`'s `(*args, **kwds)` for a
  `Generic` subclass, so `parameters['default']` raised `KeyError` on the
  KTLO leg. Read `field.__init__` instead, which every version reports alike.

Closes #1841
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 93c2a68ed. The Sonnet reviewer re-checked the Python 3.8 fix.

  • The only change is in test_no_value_reprs_as_its_own_name, which now reads inspect.signature(field.__init__). StringField.__init__ and IPv4AddressField.__init__ both take default=NO_VALUE.
  • On a real Python 3.8.20:
    • the previous head reproduces CI's KeyError: 'default';
    • with the fix, the sentinel, module, registry-protocol and construction-keyword test files all pass;
    • the 3.10 runs pass too.
  • The PR's diff uses no other 3.9+ API or syntax, and the module, including pickling at protocols 0–5, imports and passes on 3.8.

My own mutation checks from the earlier verdict still apply: allowing type attribute assignment fails 31 tests, and dropping the subclass check fails 4.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw
JarryShaw merged commit 946ab6d into main Oct 17, 2026
55 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 17, 2026
@JarryShaw
JarryShaw deleted the fix/1841-novalue-repr branch October 17, 2026 13:20
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design A design or decision issue: a pattern being decided rather than a defect or a request fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant