fix(corekit): export only the sentinel objects, not their types

[JarryShaw]

Describe the bug

The three sentinel types are exported alongside their objects, against the maintainer's ruling that only the objects should be. Measured on origin/main:

pcapkit.corekit.module.__all__         ['NULL', 'NullType']          <- type exported
pcapkit.corekit.enum.__all__           ['NO_DEFAULT', 'NoDefaultType'] <- type exported
pcapkit.corekit.fields.field.__all__   []                            <- NEITHER exported

The ruling, from #719:

we should ONLY export the objects (like NULL) to __all__, and leave the types (like NullType) out.

Definitions, for reference: NullType at pcapkit/corekit/module.py:31 with NULL at :154; NoDefaultType at pcapkit/corekit/enum.py:104 with NO_DEFAULT at :265; NoValueType at pcapkit/corekit/fields/field.py:27 with NoValue at :36.

Expected behavior

Three edits, and the third is the opposite direction from the other two — worth stating because "remove the types" is only two-thirds of it:

1. pcapkit/corekit/module.py — remove 'NullType' from __all__.
2. pcapkit/corekit/enum.py — remove 'NoDefaultType' from __all__.
3. pcapkit/corekit/fields/field.py — NoValue is not exported at all. The ruling says export the objects, so either it should be added, or it is deliberately internal and the ruling does not reach it. That needs deciding rather than assuming; see below.

This is a breaking change and should be labelled so. Removing a name from __all__ changes what from pcapkit.corekit.module import * yields, which is a public-contract change under this repository's own taxonomy. The classes themselves stay importable by name — only the star-import surface narrows.

One interaction to check before landing

tests/project/test_public_api.py asserts that public packages export everything public they hold, and #904's six red CI legs were caused by exactly that test when a public class was missing from an aggregator. Removing a type from __all__ while the class remains public may trip the same assertion — in which case the test encodes the opposite convention and needs updating alongside, with a comment naming this issue. Establish which before changing either.

Additional context

The maintainer also raised an open design question in the same comment, tracked here rather than lost:

Also considering if we should move all these sentinels to a consolidated module (with multiple sub modules) or a dedicated single file.

There are only three sentinels and they live in the modules that use them — module.py, enum.py, fields/field.py. Consolidating buys one obvious thing (a single place to read the convention, which docs/source/conventions.rst already provides in prose) and costs a re-export shim in each original location, or a breaking import change for any caller. My recommendation is to leave them where they are and revisit at five or more, but it is a judgement call and the needs: decision label is for that half, not for the __all__ fix.

docs/source/conventions.rst documents the <SENTINEL>Type naming rule and the per-sentinel differences in its "Naming a Sentinel" section; whatever is decided here should be reflected there. Note #903 is editing that file right now, so coordinate rather than collide.

[JarryShaw]

I think we have more than the listed three. The general idea is that we only expose the final objects to users.

As to where to house them, now that we have multiple, it might be a good idea to move them out from their used spaces and into a dedicated space - either spread out to one module per sentinel or one sentknel module for all.

[JarryShaw]

You are right, and my count was wrong. There are four, not three — and docs/source/conventions.rst is the reason I got it wrong, because it documents only three.

I think we have more than the listed three. The general idea is that we only expose the final objects to users.

The real population. Four sentinels following the house <SENTINEL>Type convention:

NULL        NullType        pcapkit/corekit/module.py:31, :154
NoValue     NoValueType     pcapkit/corekit/fields/field.py:27, :36
NO_DEFAULT  NoDefaultType   pcapkit/corekit/enum.py:104, :265
_Absent     _AbsentType     pcapkit/protocols/protocol.py:95          <- the one I missed

_AbsentType's own docstring shows it is a deliberate fourth rather than an accident: "A distinct class rather than a bare object so that the sentinel has a name of its own in a traceback… It follows NoValueType, which does the same job for an unset field default; this is a sibling of it rather than a reuse." It is private-named, which is why it is absent from conventions.rst and why my first sweep missed it — I filtered on capitalised names.

And one bare object() sentinel that should stay as it is: _NOT_FOUND = object() at pcapkit/utilities/compat.py:73. That sits inside the cached_property backport — vendored CPython code, so it is exempt from house convention and must not be converted. Worth stating explicitly because conventions.rst's "Why a class and not object()" section reads as a blanket rule and does not carve out ported code.

So my recommendation against consolidating was based on a wrong count, and on a threshold I set myself. I said "leave them where they are and revisit at five or more". With four house sentinels — one of them added recently and explicitly citing the convention — that threshold is effectively met, and the argument I made for leaving them does not survive its own premise. Withdrawing it.

Revised recommendation: one module for all four, not one per sentinel. Four files of roughly twenty lines each is worse than one file of eighty, and the thing worth having in one place is the convention, which only a shared module makes self-evident. pcapkit/corekit/sentinels.py sits naturally beside module.py, enum.py and fields/.

Two obstacles that decide how it is done rather than whether:

• NoValue is documented as the value of FieldBase.default, so it is part of a published contract; and _Absent is private to protocol.py. Moving either means a re-export at the original location or a breaking import change. A re-export shim keeps every caller working and costs three lines per module.
• pcapkit/corekit/module.py would import from a module that may itself want NULL — check for a cycle before committing to the layout. pylint already reports 39 cyclic-import findings in this package, so adding one would be invisible in the noise rather than caught.

On the __all__ half, your general statement resolves the question I left open. "We only expose the final objects to users" means NoValue should be exported where it currently is not — pcapkit.corekit.fields.field.__all__ lists neither it nor its type. So the edits are: drop 'NullType' and 'NoDefaultType', add 'NoValue', and leave _Absent private since it is not exposed either way.

Keeping needs: decision on one point only: one module for all four — confirm? The __all__ edits and the conventions.rst correction to four can proceed regardless.

[JarryShaw]

Splitting this: the __all__ half is in flight, the housing question stays with you.

Your general statement settles the export question outright, so a worker is implementing that now — drop NullType and NoDefaultType, add NoValue (currently exported nowhere), leave _Absent private since it is not user-facing either way. It also corrects conventions.rst from three sentinels to four and adds the carve-out for _NOT_FOUND = object() in the vendored CPython cached_property backport, which the "why a class and not object()" section currently reads as forbidding.

On housing, you offered two shapes rather than picking one, so I have left needs: decision on. My recommendation is one module for all four — pcapkit/corekit/sentinels.py — because four twenty-line files are worse than one eighty-line file, and the thing worth having in one place is the convention itself, which only a shared module makes self-evident. Two things that would shape the work rather than block it: NoValue is documented as the value of FieldBase.default so it is a published contract and needs a re-export shim at its current location, and module.py importing from a module that may itself want NULL risks a cycle that pylint's existing 39 cyclic-import findings would hide rather than catch.

One adjacent find I am not acting on without you: pcapkit/corekit/enum.py's __all__ does not list EnumLookup, which #906 split out as a public base and which conventions.rst cites as EnumLookup.get. That is a contract question about a class, not a sentinel, so it is out of scope here — say the word and it gets its own issue.

[JarryShaw]

Correcting myself: the EnumLookup point in my last comment is wrong. I measured it against a local checkout that was 12 commits behind origin/main, so I read a pre-#906 enum.py.

On current main (e38d695ae), pcapkit/corekit/enum.py:100 reads:

__all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumLookup', 'EnumRegistry']

EnumLookup is exported. #906 added it there when it split the base out. There is nothing to decide and no issue to file — withdrawn, and the worker has been told to drop that sub-task.

Two other corrections from the same stale read, so the record is right: module.py's __all__ is at line 20 and also carries ModuleDescriptor, which must survive; fields/field.py's is at line 14 and carries Field, likewise. _Absent is at protocols/protocol.py:124, not :95. The scope itself is unchanged — drop NullType and NoDefaultType, add NoValue, leave _Absent private.

The housing question is still the only thing waiting on you.

[JarryShaw]

PR up: #916 (2581c780d) for the __all__ half. Cross-review dispatched on sonnet (author opus). The housing question is untouched and still yours.

Verified by me in a fresh process rather than from the hand-back:

module.py   __all__ = ['NULL', 'ModuleDescriptor']
enum.py     __all__ = ['NO_DEFAULT', 'EnumLookup', 'EnumRegistry']
field.py    __all__ = ['NoValue', 'Field']

module import * -> ['ModuleDescriptor', 'NULL']
enum   import * -> ['EnumLookup', 'EnumRegistry', 'NO_DEFAULT']
field  import * -> ['Field', 'NoValue']
NullType / NoDefaultType / NoValueType : still importable by explicit name

So the ruling lands exactly — objects exposed, types withdrawn from import * but not from the module — and ModuleDescriptor, Field, EnumLookup and EnumRegistry all survive. No sentinels.py was created and nothing was moved, which is what I asked for.

conventions.rst is corrected from three sentinels to four, adding _Absent/_AbsentType at protocols/protocol.py:124 with the vendored-CPython carve-out for _NOT_FOUND = object().

One thing I am having the reviewer judge rather than decide myself, because it is arguably a consequence of the ruling you may not have had in view. NoValue is the one addition, and conventions.rst already records that NoValueType is not a singleton — NoValueType() is NoValue is False — and carries no __repr__ of its own. So the object now publicly exported is the one sentinel here that fails the identity guarantee the others provide. Exporting it is what "only export the objects" plainly asks for, and it is already documented as the value of FieldBase.default, so I have not held the PR over it. But if a caller writes NoValueType() expecting NoValue, they get a distinct object that fails every is check. Worth either making it a true singleton or saying in its docstring that it is not — a follow-up either way, not this PR.

Also flagged for the reviewer: whether removing the types from __all__ silently drops their Sphinx pages, since automodule with :members: follows __all__ when present. If it does, that is a fix inside this PR rather than a follow-up.

[JarryShaw]

What needs my decision exactly? And what's your recommendation?

[JarryShaw]

The decision: where the four sentinels live. You offered two shapes and I did not want to pick for you — "either spread out to one module per sentinel or one sentinel module for all." Everything else on this issue is already done and merged-ready in #916 (__all__ now exports NULL, NO_DEFAULT, NoValue and withdraws the three types; conventions.rst corrected from three sentinels to four).

My recommendation: one shared module, pcapkit/corekit/sentinels.py. Four files of ~20 lines each is worse than one of ~80, and the thing worth having in one place is the convention — <SENTINEL>Type naming, when to add __bool__, when to add __copy__/__reduce__ — which only a shared module makes self-evident to the next person adding a fifth.

Two things that shape the work rather than the choice:

• NoValue is documented as the value of FieldBase.default, so it is a published contract; _Absent is private to protocol.py. Moving either needs a re-export shim at the old location — three lines per module — or it is a breaking import change.
• pcapkit/corekit/module.py would import from a module that may itself want NULL. Check for a cycle before committing to the layout: pylint already reports 39 cyclic-import findings here, so a new one would be invisible rather than caught.

If you would rather not move them at all, that is also a defensible answer and I would take it — the naming convention is now documented in conventions.rst, which was the actual gap. The move only buys discoverability.

Say which and I will dispatch it. #916 does not depend on the answer either way.

[JarryShaw]

Okay one module for all four it is.

[JarryShaw]

Ruling recorded, verbatim:

Okay one module for all four it is.

So: pcapkit/corekit/sentinels.py holding NULL/NullType, NoValue/NoValueType, NO_DEFAULT/NoDefaultType and _Absent/_AbsentType, with re-export shims at the four current locations so no caller breaks.

Not dispatching yet, and the blocker is checkable: #916. It holds every file the move touches —

$ gh api repos/JarryShaw/PyPCAPKit/pulls/916/files --paginate -q '.[].filename'
pcapkit/corekit/enum.py
pcapkit/corekit/fields/field.py
pcapkit/corekit/module.py
...

— and it is MERGEABLE at d2011a0d6 with a GOOD TO GO verdict, waiting on you. Starting the move now would conflict on all three and force a fourth rebase of files already rebased twice today, so it waits one merge. Unblocks with:

gh pr view 916 -R JarryShaw/PyPCAPKit --json state -q .state   # MERGED

Three things already established that the work must respect, so they are not rediscovered:

• NoValue is a published contract — documented as the value of FieldBase.default — and _Absent is private to protocol.py. Both need a re-export at the original location, three lines each, or it is a breaking import change.
• Check for an import cycle before committing to the layout. module.py would import from a module that may itself want NULL. pylint already reports 39 cyclic-import findings in this package, so a new one would be invisible rather than caught — it has to be looked for deliberately.
• _NOT_FOUND = object() at pcapkit/utilities/compat.py:73 does not move. It is inside the vendored CPython cached_property backport and is exempt from the house convention; fix(corekit): export the sentinel objects, not their types #916 documents that carve-out.

Marking blocked rather than wip, since nothing is in flight. I dispatch the moment #916 lands.