From d2011a0d6e993c4072fae7171ba2c41d4af35961 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 29 Sep 2026 02:12:52 -0400 Subject: [PATCH] fix(corekit): export the sentinel objects, not their types Per the owner's ruling on #911 -- "we should ONLY export the objects (like `NULL`) to users" -- which cuts both ways: - `corekit/module.py`: drop `'NullType'` from `__all__`. - `corekit/enum.py`: drop `'NoDefaultType'` from `__all__`. - `corekit/fields/field.py`: **add** `'NoValue'`, exported neither way before. - `docs/source/conventions.rst`: the sentinel section documented three sentinels; there are four. Adds `_Absent`/`_AbsentType`, fixes both counts, records the export rule, and carves out `_NOT_FOUND = object()` in the vendored `cached_property` backport as exempt from it. - `tests/const/test_const_registry_protocol.py`: the one test asserting `'NoDefaultType' in enum.__all__` now asserts the opposite. Breaking only for `import *`: every type stays importable by its dotted path, which is how every in-tree user already names them. `tests/corekit/` and `tests/project/test_public_api.py` pass; Sphinx warnings unchanged at 58. --- docs/source/contributing/conventions.rst | 66 +++- pcapkit/corekit/enum.py | 2 +- pcapkit/corekit/fields/field.py | 2 +- pcapkit/corekit/module.py | 2 +- tests/const/test_const_registry_protocol.py | 13 +- tests/corekit/test_sentinel_exports_unit.py | 394 ++++++++++++++++++++ 6 files changed, 461 insertions(+), 18 deletions(-) create mode 100644 tests/corekit/test_sentinel_exports_unit.py diff --git a/docs/source/contributing/conventions.rst b/docs/source/contributing/conventions.rst index cfc100e86c..6a61d23fd1 100644 --- a/docs/source/contributing/conventions.rst +++ b/docs/source/contributing/conventions.rst @@ -137,7 +137,7 @@ caller might legitimately pass. The house rule, from the maintainer: Keep the sentinel object's type class naming as ``Type``. That is, the class takes the instance's name in CamelCase with ``Type`` appended. The -three in the tree follow it: +four in the tree follow it: .. list-table:: :header-rows: 1 @@ -155,18 +155,46 @@ three in the tree follow it: * - ``NO_DEFAULT`` - ``NoDefaultType`` - :mod:`pcapkit.corekit.enum` + * - ``_Absent`` + - ``_AbsentType`` + - :mod:`pcapkit.protocols.protocol` Note what the rule does **not** fix: the **instance** name's casing is deliberately free, which is why ``NULL`` and ``NoValue`` disagree and both are correct. Pick whichever reads better at the call site, and where a name already exists, keep it -- renaming a published sentinel costs every caller for no gain. +Nor does it fix the **leading underscore**. ``_Absent`` is private -- it is read in +``_declared_keywords`` and discarded there, never leaving +:mod:`pcapkit.protocols.protocol` -- and it is still held to the convention, which is +why its type is ``_AbsentType`` and not ``_Absent_t`` or ``Absent``. It is a +deliberate fourth rather than an accident, and its own docstring +(:file:`pcapkit/protocols/protocol.py`, line 95) says so: + + A distinct class rather than a bare :obj:`object` so that the sentinel has a name + of its own in a traceback or a debugger, and so that a type checker has something + to name where ``object()`` would give it nothing. It follows + :class:`~pcapkit.corekit.fields.field.NoValueType`, which does the same job for an + unset field default; this is a sibling of it rather than a reuse [...] + +The private name is also why this table listed three for as long as it did: a sweep +filtered on capitalised names does not see it. When adding a sentinel, add it here +whether or not it is public. + +What reaches users is the **object only**. The maintainer's ruling: *"we should ONLY +export the objects (like* ``NULL`` *) to users"* -- so a public sentinel names its +instance in its module's ``__all__`` and leaves the type out of it (GitHub issue #911). +The type stays importable by its dotted path, for an annotation or an ``is`` guard; it +is ``import *`` that no longer offers it. A private sentinel such as ``_Absent`` is in +neither, which is what private means here. + .. note:: - Of the three, only :class:`~pcapkit.corekit.module.NullType` is a full worked + Of the four, only :class:`~pcapkit.corekit.module.NullType` is a full worked example. ``NoValueType`` follows the naming rule but is **not** a singleton (``NoValueType() is NoValue`` is :obj:`False`) and has no ``__repr__`` of its own, - so it demonstrates the name and nothing else. Copy ``NullType`` when you need a + so it demonstrates the name and nothing else; ``_AbsentType`` has a ``__repr__`` + (````) but no singleton guard either. Copy ``NullType`` when you need a pattern to follow. Why a class and not ``object()`` @@ -192,33 +220,45 @@ appears in a signature, in :func:`help` output and in a traceback. Compare what identity ``__eq__`` and is exactly as safe as ``object()``. That argument appeared in an early draft of :mod:`pcapkit.corekit.enum` and was wrong. +**Ported code is exempt.** ``_NOT_FOUND = object()`` at +:file:`pcapkit/utilities/compat.py`, line 73, sits inside the ``cached_property`` +backport taken for interpreters below 3.8, which tracks CPython's own +:mod:`functools` implementation down to that name. It is **not** to be converted: the +value of a vendored backport is that it can still be diffed against upstream, and a +house-style rewrite destroys that in exchange for a sentinel nobody outside those forty +lines ever sees. The rule above is for sentinels this package writes itself. + What to implement, and what not to ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -The three sentinels deliberately differ, and the differences are **needs, not +The four sentinels deliberately differ, and the differences are **needs, not inconsistencies**: ``__new__`` returning a cached instance Guards against a caller constructing a second, non-identical sentinel that then - fails every ``is`` check. Worth having wherever the type is exported. + fails every ``is`` check. Worth having wherever the type is reachable by a caller at + all -- which, since the type is kept out of ``__all__``, means wherever it is + importable by its dotted path rather than wherever it is star-exported. :class:`~pcapkit.corekit.module.NullType` documents the limit honestly: a module **reload** re-executes the class statement, so the guard does not survive one, and code holding the pre-reload instance will fail ``is``. ``__bool__`` returning :obj:`False` - ``NULL`` and ``NoValue`` have it, because each stands for an *absent value* and - reads naturally in a boolean test. ``NO_DEFAULT`` deliberately does **not**: it is a - marker meaning *no default was supplied*, it is only ever tested with ``is``, and - making it falsy would invite ``if not default:`` -- which would then treat a - caller's genuine falsy default (``0``, ``''``, :obj:`None`, :obj:`False`) the same - as the sentinel, the very confusion the sentinel exists to prevent. + ``NULL``, ``NoValue`` and ``_Absent`` have it, because each stands for an *absent + value* and reads naturally in a boolean test. ``NO_DEFAULT`` deliberately does + **not**: it is a marker meaning *no default was supplied*, it is only ever tested + with ``is``, and making it falsy would invite ``if not default:`` -- which would + then treat a caller's genuine falsy default (``0``, ``''``, :obj:`None`, + :obj:`False`) the same as the sentinel, the very confusion the sentinel exists to + prevent. ``__copy__`` / ``__deepcopy__`` / ``__reduce__`` :class:`~pcapkit.corekit.module.NullType` has them because ``NULL`` is stored in a :class:`~pcapkit.corekit.module.ModuleDescriptor` field, so a caller's :func:`copy.deepcopy` or :mod:`pickle` can walk into it and would otherwise - reconstruct a second instance. ``NO_DEFAULT`` has none, because it is never stored - in any structure a caller copies -- it only ever appears as a default argument. + reconstruct a second instance. ``NO_DEFAULT`` and ``_Absent`` have none, because + neither is ever stored in any structure a caller copies -- one only ever appears as + a default argument, and the other never leaves the module that reads it. Add them when, and only when, the sentinel becomes reachable from something copyable. diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 0e8ae2b21d..15b4795ba7 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -97,7 +97,7 @@ from typing_extensions import Self -__all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumLookup', 'EnumRegistry'] +__all__ = ['NO_DEFAULT', 'EnumLookup', 'EnumRegistry'] @final diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 77c63436d7..4726535e73 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -11,7 +11,7 @@ from pcapkit.utilities.compat import final from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue, ProtocolError -__all__ = ['Field'] +__all__ = ['NoValue', 'Field'] if TYPE_CHECKING: from typing import IO, Any, Callable, Iterator, Optional diff --git a/pcapkit/corekit/module.py b/pcapkit/corekit/module.py index 9feb8d5710..773fa1a133 100644 --- a/pcapkit/corekit/module.py +++ b/pcapkit/corekit/module.py @@ -17,7 +17,7 @@ from pcapkit.utilities.compat import final from pcapkit.utilities.exceptions import ProtocolError -__all__ = ['NULL', 'NullType', 'ModuleDescriptor'] +__all__ = ['NULL', 'ModuleDescriptor'] if TYPE_CHECKING: from typing import Any, Callable, Type diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index 16187ab7b5..ddd959e935 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -1356,12 +1356,21 @@ def test_repr_is_the_readable_form_not_a_bare_object_address(self) -> None: self.assertEqual(rendered, '') self.assertNotIn('0x', rendered) - def test_type_is_the_dedicated_sentinel_class_and_both_are_exported(self) -> None: + def test_type_is_the_dedicated_sentinel_class_and_only_the_object_is_exported(self) -> None: + """GitHub issue #911 reversed half of what this used to assert. + + It read ``assertIn('NoDefaultType', enum_module.__all__)`` -- the type + *and* the object were exported. The owner's ruling: *"we should ONLY + export the objects (like* ``NULL`` *) to users"*, so the type is out of + :attr:`__all__` while staying importable by its dotted path, which is + what the last assertion here pins. + """ import pcapkit.corekit.enum as enum_module self.assertIs(type(enum_module.NO_DEFAULT), enum_module.NoDefaultType) self.assertIn('NO_DEFAULT', enum_module.__all__) - self.assertIn('NoDefaultType', enum_module.__all__) + self.assertNotIn('NoDefaultType', enum_module.__all__) + self.assertTrue(hasattr(enum_module, 'NoDefaultType')) def test_constructing_the_type_again_returns_the_same_instance(self) -> None: """The ``__new__`` singleton guard: a caller who does not realise diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py new file mode 100644 index 0000000000..314a59dd52 --- /dev/null +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -0,0 +1,394 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #911: a sentinel exports its **object**, never its type. + +The owner's ruling, verbatim: *"One thing about sentinel types and objects in the +library: we should ONLY export the objects (like* ``NULL`` *) to users."* That is +one rule with two directions, and the tree on ``origin/main`` broke it in both: + +* ``pcapkit.corekit.module.__all__`` was ``['NULL', 'NullType', 'ModuleDescriptor']`` + -- the type is exported; +* ``pcapkit.corekit.enum.__all__`` was + ``['NO_DEFAULT', 'NoDefaultType', 'EnumLookup', 'EnumRegistry']`` -- likewise; +* ``pcapkit.corekit.fields.field.__all__`` was ``['Field']`` -- the *object* is + missing, so this one needs a name added rather than removed. + +So two names come out and one goes in, which is why the fix is not "remove the +types". :class:`SentinelExportTests` asserts the shape in both directions and per +module, rather than as one aggregate that a half-applied change would still satisfy. + +What only ``import *`` narrows: the type stays importable by its dotted path, so +``from pcapkit.corekit.module import NullType`` keeps working and an annotation +naming it keeps resolving. That is the whole extent of the breaking change the issue +is labelled for, and +:meth:`SentinelExportTests.test_every_sentinel_type_is_still_importable_by_name` +pins it so a later reading of the ruling cannot escalate into deleting the types. + +The population is **four**, not the three +:file:`docs/source/conventions.rst` documented -- ``_Absent`` / +``_AbsentType`` in :mod:`pcapkit.protocols.protocol` is the fourth, missed because a +sweep filtered on capitalised names does not see a leading underscore. +:class:`SentinelPopulationTests` pins the count and the doc together, so the next +sentinel cannot be added to one without the other. ``_Absent`` is private and stays +out of :attr:`__all__` in both directions, which is what the ruling means by "to +users". + +One sentinel is deliberately **not** held to any of this: ``_NOT_FOUND = object()`` +at :file:`pcapkit/utilities/compat.py`, line 73, inside the ``cached_property`` +backport for interpreters below 3.8. It is ported code and exempt, and +:meth:`SentinelPopulationTests.test_the_vendored_bare_object_sentinel_is_left_alone` +pins that as an exemption rather than leaving it to read as an oversight against the +"why a class and not ``object()``" rule. + +On the tree before this change, four of these fail: + +* ``test_module_exports_the_object_and_not_the_type`` -- ``'NullType'`` is in + :attr:`__all__`; +* ``test_enum_exports_the_object_and_not_the_type`` -- ``'NoDefaultType'`` is; +* ``test_field_exports_the_object_and_not_the_type`` -- ``'NoValue'`` is not; +* ``test_star_import_binds_the_objects_and_not_the_types`` -- all three of the above, + through the one operation that actually reads :attr:`__all__`. + +and two more fail on the documentation half: + +* ``test_conventions_doc_lists_every_sentinel_in_the_tree`` -- the table has three + rows and the prose says "three"; +* ``test_conventions_doc_carves_out_the_vendored_bare_object`` -- the carve-out is + not there at all. + +The rest pass either way and are regression guards: the sibling exports that must +survive the edit (``ModuleDescriptor``, ``Field``, ``EnumLookup``, +``EnumRegistry``), the types staying importable, and the naming convention itself. + +""" +from __future__ import annotations + +import pathlib +import re +import unittest + +from pcapkit.corekit.enum import NO_DEFAULT, NoDefaultType +from pcapkit.corekit.fields.field import NoValue, NoValueType +from pcapkit.corekit.module import NULL, NullType +from pcapkit.protocols.protocol import _Absent, _AbsentType + +#: Repository root, for the two tests that read a file rather than import it. +ROOT = pathlib.Path(__file__).resolve().parents[2] + +#: Every sentinel in the tree that follows the house ``Type`` convention, +#: as ``(instance name, instance, type, module)``. Four, not the three +#: :file:`docs/source/conventions.rst` used to document -- see the module docstring. +SENTINELS = ( + ('NULL', NULL, NullType, 'pcapkit.corekit.module'), + ('NoValue', NoValue, NoValueType, 'pcapkit.corekit.fields.field'), + ('NO_DEFAULT', NO_DEFAULT, NoDefaultType, 'pcapkit.corekit.enum'), + ('_Absent', _Absent, _AbsentType, 'pcapkit.protocols.protocol'), +) + +#: The public three of :data:`SENTINELS`, as ``(module, object name, type name)``. +#: ``_Absent`` is absent from it deliberately: it is private, so it is exported +#: neither way and the export rule does not reach it. +PUBLIC_SENTINELS = ( + ('pcapkit.corekit.module', 'NULL', 'NullType'), + ('pcapkit.corekit.fields.field', 'NoValue', 'NoValueType'), + ('pcapkit.corekit.enum', 'NO_DEFAULT', 'NoDefaultType'), +) + + +def _expected_type_name(instance_name: 'str') -> 'str': + """The type name the house convention derives from an instance name. + + ``NULL`` gives ``NullType`` and ``NO_DEFAULT`` gives ``NoDefaultType``, so an + all-caps name is title-cased word by word. ``NoValue`` is already CamelCase and + is left as it is -- ``str.capitalize`` would lowercase its tail into + ``Novalue``. A leading underscore is carried through, which is what makes + ``_Absent`` give ``_AbsentType`` rather than ``AbsentType``. + + """ + lead = '_' if instance_name.startswith('_') else '' + bare = instance_name.lstrip('_') + if bare.isupper(): + bare = ''.join(word.capitalize() for word in bare.split('_')) + return f'{lead}{bare}Type' + + +def _star_import(module: 'str') -> 'dict[str, object]': + """The namespace ``from import *`` binds. + + The only operation that actually reads :attr:`__all__`, which is why the export + rule is asserted through it and not only against the list literal. + + """ + namespace = {} # type: dict[str, object] + exec(f'from {module} import *', namespace) # pylint: disable=exec-used + return namespace + + +def _sentinel_section() -> 'str': + """The "Naming a sentinel" section of :file:`docs/source/conventions.rst`. + + Sliced by its own section markers rather than by line number, so an edit + elsewhere in the file -- or the move to + :file:`docs/source/contributing/conventions.rst` that GitHub pull request #912 + is making -- does not silently make this read the wrong text. + + """ + for candidate in ('docs/source/conventions.rst', + 'docs/source/contributing/conventions.rst'): + path = ROOT / candidate + if path.is_file(): + break + else: # pragma: no cover + raise AssertionError('conventions.rst not found under docs/source') + + text = path.read_text(encoding='utf-8') + start = text.index('.. _sentinel-convention:') + end = text.index('.. _registry-protocol:', start) + return text[start:end] + + +class SentinelExportTests(unittest.TestCase): + """``__all__`` names the object and not the type, per module.""" + + def test_module_exports_the_object_and_not_the_type(self) -> 'None': + """``['NULL', 'NullType', 'ModuleDescriptor']`` loses the middle entry. + + ``ModuleDescriptor`` is not a sentinel and has to survive the edit, which + is asserted here rather than in a separate test because removing it is the + plausible way to get this line wrong. + + """ + import pcapkit.corekit.module as module + + self.assertIn('NULL', module.__all__) + self.assertNotIn('NullType', module.__all__) + self.assertIn('ModuleDescriptor', module.__all__) + + def test_enum_exports_the_object_and_not_the_type(self) -> 'None': + """``EnumLookup`` and ``EnumRegistry`` are not sentinels and stay. + + ``EnumLookup`` in particular: GitHub issue #906 split it out as a public + base and :file:`docs/source/conventions.rst` cites its ``get``, so dropping + it while removing the sentinel type next to it would break that reference. + + """ + import pcapkit.corekit.enum as enum + + self.assertIn('NO_DEFAULT', enum.__all__) + self.assertNotIn('NoDefaultType', enum.__all__) + self.assertIn('EnumLookup', enum.__all__) + self.assertIn('EnumRegistry', enum.__all__) + + def test_field_exports_the_object_and_not_the_type(self) -> 'None': + """The direction that is an *addition*: ``NoValue`` was exported by neither. + + :attr:`FieldBase.default ` + is documented as being this object, so the ruling reaches it: a value a + caller is told to compare against is a value ``import *`` should provide. + + """ + import pcapkit.corekit.fields.field as field + + self.assertIn('NoValue', field.__all__) + self.assertNotIn('NoValueType', field.__all__) + self.assertIn('Field', field.__all__) + + def test_star_import_binds_the_objects_and_not_the_types(self) -> 'None': + """Through ``import *`` itself, which is the surface the ruling is about.""" + for module, obj, type_ in PUBLIC_SENTINELS: + namespace = _star_import(module) + with self.subTest(module=module): + self.assertIn(obj, namespace) + self.assertNotIn(type_, namespace) + + def test_star_import_hands_back_the_canonical_object(self) -> 'None': + """Not merely *a* binding of that name -- the one sentinel instance. + + A star-import that bound a second, non-identical object would satisfy the + test above and break every ``is`` check the sentinel exists for. + + """ + for module, obj, _ in PUBLIC_SENTINELS: + namespace = _star_import(module) + with self.subTest(module=module): + expected = next(instance for name, instance, _, where in SENTINELS + if name == obj and where == module) + self.assertIs(namespace[obj], expected) + + def test_every_sentinel_type_is_still_importable_by_name(self) -> 'None': + """The exact extent of the breaking change: ``import *`` narrows, nothing else. + + Leaving :attr:`__all__` is not being made private. An annotation or an + ``isinstance`` check that names the type keeps working, and the tree itself + relies on that -- this module's own imports are the demonstration. + + """ + for name, instance, type_, module in SENTINELS: + with self.subTest(sentinel=name): + self.assertIs(type(instance), type_) + self.assertEqual(type_.__module__, module) + + def test_the_private_sentinel_is_exported_neither_way(self) -> 'None': + """``_Absent`` is private, so the export rule does not reach it. + + Pinned rather than assumed: the ruling says *export the objects*, and a + literal reading of that would add ``_Absent`` to + :attr:`pcapkit.protocols.protocol.__all__`, which would publish a sentinel + whose own docstring says it never leaves the module. + + """ + import pcapkit.protocols.protocol as protocol + + self.assertNotIn('_Absent', protocol.__all__) + self.assertNotIn('_AbsentType', protocol.__all__) + self.assertNotIn('_Absent', _star_import('pcapkit.protocols.protocol')) + + +class SentinelPopulationTests(unittest.TestCase): + """There are four, they follow the naming rule, and the docs say so.""" + + def test_every_sentinel_follows_the_naming_convention(self) -> 'None': + """*"Keep the sentinel object's type class naming as* ``Type``*."*""" + for name, _, type_, _ in SENTINELS: + with self.subTest(sentinel=name): + self.assertEqual(type_.__name__, _expected_type_name(name)) + + def test_conventions_doc_lists_every_sentinel_in_the_tree(self) -> 'None': + """The doc said "three" and listed three; ``_Absent`` was the fourth. + + Asserting the names rather than only the count, because a count corrected + without the row -- or a row added without the count -- is the same defect + in a different place. + + """ + section = _sentinel_section() + + for name, _, type_, module in SENTINELS: + with self.subTest(sentinel=name): + self.assertIn(f'``{name}``', section) + self.assertIn(f'``{type_.__name__}``', section) + self.assertIn(module, section) + + self.assertIn('four in the tree follow it', section) + self.assertNotIn('three in the tree follow it', section) + self.assertNotIn('The three sentinels deliberately differ', section) + + def test_conventions_doc_records_that_only_the_object_is_exported(self) -> 'None': + """The ruling this change implements belongs in the doc that states the rule.""" + section = _sentinel_section() + + self.assertIn('ONLY', section) + self.assertIn('#911', section) + + def test_conventions_doc_carves_out_the_vendored_bare_object(self) -> 'None': + """"Why a class and not ``object()``" read as a blanket rule with no exception.""" + section = _sentinel_section() + + self.assertIn('_NOT_FOUND', section) + self.assertIn('pcapkit/utilities/compat.py', section) + + def test_the_vendored_bare_object_sentinel_is_left_alone(self) -> 'None': + """The carve-out, asserted against the source it carves out. + + Read rather than imported: the backport is inside ``if sys.version_info < + (3, 8):``, so on any interpreter that runs this suite the branch is dead and + ``_NOT_FOUND`` is never bound. A test that imported it would pass + vacuously. + + """ + source = (ROOT / 'pcapkit' / 'utilities' / 'compat.py').read_text(encoding='utf-8') + self.assertRegex(source, r'(?m)^\s+_NOT_FOUND = object\(\)$') + + def test_the_sentinel_table_has_a_row_per_sentinel_and_no_more(self) -> 'None': + """Counted from the table itself, so a fifth sentinel cannot be half-added.""" + section = _sentinel_section() + table = section[section.index('.. list-table::'):] + table = table[:table.index('\n\n', table.index('- Defined in'))] + + rows = re.findall(r'^ \* - (\S+)$', table, re.MULTILINE) + self.assertEqual(rows, ['Instance'] + [f'``{name}``' for name, _, _, _ in SENTINELS]) + + +class SentinelBehaviourTests(unittest.TestCase): + """The per-sentinel differences :file:`docs/source/conventions.rst` documents. + + Not part of the export change, and asserted here because the doc edit that goes + with it makes claims about all four -- an undocumented ``__bool__`` or a missing + ``__repr__`` would make the corrected prose wrong in a way no other test sees. + + """ + + def test_the_absent_value_sentinels_are_falsy(self) -> 'None': + """``NULL``, ``NoValue`` and ``_Absent`` each stand for an absent value.""" + for sentinel in (NULL, NoValue, _Absent): + with self.subTest(sentinel=repr(sentinel)): + self.assertFalse(sentinel) + + def test_no_default_is_deliberately_truthy(self) -> 'None': + """It means *no default was supplied*, and is only ever tested with ``is``. + + Falsy would invite ``if not default:``, which would then read a caller's + genuine ``0`` or ``''`` as the sentinel -- the confusion it exists to + prevent. + + """ + self.assertTrue(NO_DEFAULT) + + def test_the_private_sentinel_reprs_as_its_own_name(self) -> 'None': + """````, not ````. + + The reason the house rule prefers a class at all, and the reason the doc + can now name ``_AbsentType`` as having a ``__repr__`` where ``NoValueType`` + does not. + + """ + self.assertEqual(repr(_Absent), '') + self.assertNotIn('0x', repr(_Absent)) + self.assertNotIn('__repr__', vars(NoValueType)) + + +class NoValueIsTheDocumentedFieldDefaultTests(unittest.TestCase): + """Why the ruling reaches ``NoValue`` at all. + + ``NoValue``'s own comment in :mod:`pcapkit.corekit.fields.field` reads *"Default + value for* :attr:`FieldBase.default `*"*, + so it is the value a caller is told to compare a field's default against -- which + is what makes withholding it from ``import *`` the defect rather than a + preference. That contract had no test: the ``default`` setter and deleter were + both uncovered, and the deleter is the only code path that puts the sentinel + *back*. + + :class:`~pcapkit.corekit.fields.strings.BytesField` is the concrete field under + test, following :file:`tests/corekit/test_fields_field.py`: a real user-facing + field type that inherits ``default`` unchanged, rather than a hand-rolled + stand-in. + + """ + + def test_an_undefaulted_field_reports_the_sentinel(self) -> 'None': + """``is``, not ``==`` -- the whole point of a sentinel.""" + from pcapkit.corekit.fields.strings import BytesField + + self.assertIs(BytesField(length=4).default, NoValue) + + def test_setting_and_deleting_a_default_round_trips_through_the_sentinel(self) -> 'None': + """Deleting a default restores ``NoValue``, rather than :obj:`None` or ``b''``. + + :obj:`None` and ``b''`` are both values a caller may legitimately want as a + default, so either would be indistinguishable from "no default given" -- + exactly the confusion the sentinel exists to prevent. + + """ + from pcapkit.corekit.fields.strings import BytesField + + field = BytesField(length=4) + field.default = b'\x00\x01\x02\x03' + self.assertEqual(field.default, b'\x00\x01\x02\x03') + self.assertIsNot(field.default, NoValue) + + del field.default + self.assertIs(field.default, NoValue) + self.assertFalse(field.default) + + +if __name__ == '__main__': + unittest.main()