Conversation
The bullet #667 would otherwise have carried, kept here so that #667 touches only `pcapkit/corekit/multidict.py` and `tests/corekit/test_multidict.py`. One bullet, because it is one convention gap in one class: `_Missing` behind `MultiDict.pop` and `OrderedMultiDict.pop` lacked the `@final` and the falsy `__bool__` that `NoValueType` in `pcapkit.corekit.fields.field` sets as the package's convention for a marker of this kind. The bullet says plainly that no behaviour changes, and says why rather than asserting it: both `pop()` implementations decide by identity, never by truthiness, and `pop()` structurally cannot return the marker -- it returns `default` only on the branch where `default is not _missing`. It also names the one way the old truthiness was observable, which is what justifies touching it at all: `inspect.signature(MultiDict.pop).parameters['default'].default` hands the marker to any caller who asks, and `if default:` on it reported "a default was supplied" where none had been. It closes by recording the disposition of the other two sites from the #640 sweep, so the entry is the whole story: site 1 needed nothing, and `_NOT_FOUND` in `pcapkit.utilities.compat` stays a bare `object()` deliberately, being a verbatim line of CPython's `functools.cached_property` inside a `sys.version_info < (3, 8)` branch no supported interpreter reaches. The reasoning behind that one is on #661, not here. `:obj:` roles had to come out: `util/changelog_md.py` rejects them with `ResidualMarkupError`, since its six conversion rules do not cover interpreted text and `CHANGELOG.md` would carry the role through as literal text. Double backticks instead, which is what the rest of the entry file uses. 20 lines added to the entry file; `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on e55ba36 and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree at a stale f846523 and could not be taken here. Note e55ba36, not the 69a6e13 I was given: the branch had already moved on with #665's and #651's entries. Refs #661
…bool__` (#661) The sweep in #661 found `_missing` already a purpose-built class rather than a bare `object()`, but short of the convention `NoValueType` sets at `pcapkit/corekit/fields/field.py:26-36`. * A marker meaning *no `default` was supplied* answered `True` to `bool()`, stating the opposite of what it means. It is a private name, but its value is not hidden: it is the runtime default of `MultiDict.pop` and `OrderedMultiDict.pop`, so both `inspect.signature(MultiDict.pop).parameters['default'].default` and plain `MultiDict.pop.__defaults__` hand it to a caller who asks. Now defines `__bool__` returning `False`. * `_Missing` was subclassable, which the convention forbids for a singleton marker. Now carries `@final`, from `pcapkit.utilities.compat`. * Documented the class and all three dunders, including why `None` cannot serve as the marker and why `__reduce__` returns a bare name -- and wired them into `docs/source/pcapkit/corekit/multidict.rst`, which had no directive for `_Missing` at all, so the new docstrings would have been unreachable by any doc build. The `Auxiliaries` section mirrors the one `docs/source/pcapkit/corekit/fields/field.rst:15-19` gives `NoValueType`/`NoValue`. Both `pop()` implementations decide on identity (`is not _missing`), never on truthiness, so the falsy marker changes no behaviour: `repr`, `__reduce__`, the pickle bytes at every protocol 0-5, both signature defaults, and every `pop()` result including the falsy defaults `None`/`False`/`0`/`''`/`[]` are unchanged. A fresh process that has not imported the module still unpickles the marker to its own `_missing`. `tests/corekit/`: 169 passed, 398 subtests; `tests/project/`: 96 passed, 469 subtests. `multidict.py` stays at 100% statement and branch coverage (284 -> 288 statements, 0 missed either side). The added rst builds clean under `-W`. Refs #661
e101851 to
064ad15
Compare
Cross-review verdictGOOD TO GO — with one required change, which has been made. That is the reviewer's verdict verbatim: it opened Cross-review ran on Sonnet (this change was authored on Opus), briefed to falsify rather than The required change, and what was done
Fixed: an What the reviewer independently reproducedAll eight remaining claims verified TRUE, each with evidence it obtained itself rather than
Where it disputed the PR, and what changed as a resultTwo things, both accepted:
It also pushed back on the What it could not verifyA full Nits it raised and I did not act on
One thing the reviewer observed that was meIt reported this worktree's |
Closing:
|
Site 2 of the three sentinel sites the sweep in #661 left open. Site 1 (
NoValue) needs nothingand is untouched. Site 3 (
_NOT_FOUND) is a decision rather than a change, and the reasoning isposted on #661 rather
than acted on here.
What was short of the convention
_missingwas already what the review comment on #640 asked for — an instance of a purpose-builtclass, not a bare
object(). Measured againstNoValueTypeatpcapkit/corekit/fields/field.py:26-36,which is the package's convention for a sentinel of this kind, it lacked exactly two things:
@finaland__bool__.Truetobool(). A marker meaning nodefaultwas supplied that is truthystates the opposite of what it means.
NoValueTypedefines__bool__returningFalseforprecisely that reason.
_Missingwas subclassable. The convention marks a singleton marker@final; nothingsubclasses it, and nothing should.
The issue's own recommendation was
@finalonly, on the grounds that__bool__"would also bea live behaviour change rather than a cosmetic one, since this instance is reachable by callers".
That reservation is worth taking seriously, and the measurements below are what settle it — the
deciding fact is that
_missingcan never escape through the public API:pop()returnsdefaultonly on the branch wheredefault is not _missing, so no caller can ever receive it,including one that passes
_missingitself asdefault(that still raises). The only routes to itstruthiness are deliberate introspection —
inspect.signature(MultiDict.pop).parameters['default'].default,or plain
MultiDict.pop.__defaults__, which needs no import at all. On those routesFalseis themore correct answer, because
if default:currently reports "there is a default" when there isnot. If you would still rather have
@finalalone, drop the__bool__method and the threeassertions naming it; the rest of the change stands unchanged.
_missingis a private name by every normal measure — underscore-prefixed, absent from__all__,previously undocumented. The claim here is narrower than "it is public": it is that its value is
handed out by a public signature, which is what makes its truthiness worth being correct.
What was measured before touching it
All four
_missingsites, and none tests truthiness. Two are parameter defaults(
multidict.py:373,:592pre-change) and two are identity tests(
:393,:596— bothif default is not _missing:). Nothing anywhere doesif default:, and thereis no fifth reference in
pcapkit/,tests/,docs/orexamples/.Nothing subclasses
_Missing, package-wide or undertests/. The only references outside its owndefinition are the four sites above plus two assertions in
tests/corekit/test_multidict.py:34-35.The pickle round trip still works, at every protocol —
__reduce__returning the bare string_missingis pickle-by-name, and it is the reason the singleton survives a process boundary. Theemitted bytes are byte-identical before and after:
And across a genuine process boundary, which is the case the docstring claims and the unit test
cannot reach. Dumped in one interpreter, loaded in a second that had not imported the module:
A before/after probe of every observable differs on exactly three lines — the three that are the
point of the change:
repr,__reduce__, the pickle bytes at protocols 0 through 5, bothpop()signature defaults, andevery
pop()result — including the falsy defaultsNone,False,0,'',[], and a second_Missing()instance that is falsy but not identical — are unchanged.The
@finalassertion is not vacuoustyping.finalonly began recording__final__on the decorated class in Python 3.11, andpcapkit.utilities.compat.finalistyping.finalon everything from 3.8 up. A bareassertTrue(hasattr(_Missing, '__final__'))would therefore pass vacuously on 3.8 through 3.10,which are inside the declared support range. The test probes the decorator with a throwaway class
and skips with a named reason where it cannot record the mark, rather than asserting nothing.
Docs
docs/source/pcapkit/corekit/multidict.rsthad no directive for_Missingat all, so the newdocstrings would have been unreachable by any doc build — a self-contradiction, since the class
docstring says it follows
NoValueType, which is wired in atdocs/source/pcapkit/corekit/fields/field.rst:15-19. It now carries anAuxiliariessectionmirroring that one. The
.. automethod::entries for the two dunders follow this package's ownidiom —
docs/source/pcapkit/corekit/context.rst:75already documents a__bool__that way.The whole-project docs build takes over nine minutes, which is too slow to gate a four-line rst
addition, so the section was validated with a minimal single-file Sphinx project under
-W(warningsas errors): exit 0, with the class docstring, both dunder docstrings and the
#:comment on_missingall rendering and every role resolving.Verification
Failing first, then passing, pytest's exit code read from a file rather than off a pipeline, and
pcapkit.__file__asserted into the worktree with__editable__*stripped fromsys.meta_pathbefore any other import. (Those finders are classes, so
type(f).__module__isbuiltinsand afilter written that way silently misses them.)
0c7f2b7c92 failed, 9 passed—AssertionError: True is not False, thenAttributeError: type object '_Missing' has no attribute '__final__'111 passed, 20 subtests passed0tests/corekit/in full169 passed, 398 subtests passed0tests/project/in full96 passed, 469 subtests passed0Coverage does not go backwards —
pcapkit/corekit/multidict.pyis at 100% statement and branchon both sides, 284 → 288 statements with 0 missed and 0 partial branches either way. The four added
statements are the import, the
@finalline,def __bool__, and itsreturn False.pylint,mypy,isort,verminandbanditon the changed file report the same findings beforeand after: mypy's 10 pre-existing
unused-ignore/overrideerrors map one-to-one onto the new linenumbers shifted by exactly the 41 lines added, pylint's 4 findings are all outside the diff hunks
(lines 187/429/444/508), vermin's single finding is the untouched
typing_extensionsimport, andbandit is clean. No new finding was introduced.
A pre-existing failure found on the way, not caused by this change
Running
tests/corekit/test_multidict.pyandtests/project/in the same process fails threetests/project/test_public_api.pytests. Confirmed pre-existing: reproduced at untouchedorigin/main(0c7f2b7c9) in a throwaway worktree, same three failures, exit 1 — and reproduced onthis branch with both new tests
--deselected. The cause is thatbootstrap_core_modules()→ensure_package()intests/_support.pyinstalls bare stubpcapkit,pcapkit.corekitandpcapkit.utilitiesmodules intosys.modulesand nothing restores them, so a later test thatintrospects the real public surface sees the stubs (
these public packages declare no __all__). Thefull suite is green because an intervening test directory re-imports the real package, which is why
this only surfaces when those two paths are run adjacently. Reported rather than fixed here, since
it belongs in its own change.
Label
fix— notfeat(no new capability), notdocs(there is a code change), nottest(the test isevidence, not the point).
breakingwas considered and rejected:_missingis not in__all__, andpop()structurally cannot return it, so no public contract changes. The one thing that genuinelyflips is
bool()of the sentinel itself on the two introspection routes above — obscure, but real,so
breakingis additive and can be added alongsidefixif you read that reachability differently.Refs #661