Skip to content

docs: set PCAPKIT_SPHINX before importing pcapkit, not after - #520

Merged
JarryShaw merged 3 commits into
mainfrom
fix/sphinx-type-checking-env-order
Sep 19, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
fix/sphinx-type-checking-env-order

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

One line, 96 warnings, 89 documented members restored to the built HTML.

The defect

pcapkit/utilities/logging.py:73 computes the flag at import time:

SPHINX_TYPE_CHECKING = BOOLEAN_STATES.get(os.environ.get('PCAPKIT_SPHINX', 'false').casefold(), False)

docs/source/conf.py did import pcapkit at line 23 and only set os.environ['PCAPKIT_SPHINX'] = '1' at line 29. Importing pcapkit already pulls in the schema modules, so every if SPHINX_TYPE_CHECKING: TypedDict was never defined — and the .rst pages that autodoc them emitted autodoc: failed to import ….

Measured, both orderings

Against pcapkit/protocols/schema/application/httpv2.py, whose FrameFlags sits under that guard at line 36:

set AFTER import  -> SPHINX_TYPE_CHECKING = False | has FrameFlags: False
set BEFORE import -> SPHINX_TYPE_CHECKING = True  | has FrameFlags: True

And re-executing conf.py's own import prologue with the fix applied:

with the fix -> SPHINX_TYPE_CHECKING = True | has FrameFlags: True

Scope

14 modules carry if SPHINX_TYPE_CHECKING:, holding roughly 87 TypedDict classes between them — FrameFlags, StreamID, the 40+ *MessageFlags in schema/internet/mh.py, TCIType, EPBFlags and the rest. All of those names exist; none were stale, so this is purely a build-configuration defect rather than a documentation gap.

Alongside the 96 failed to import warnings, this also resolves the 2 genuine sphinx_autodoc_typehints.forward_reference warnings for TCIType and Locator, which had the same cause.

Why a comment rather than just a moved line

The ordering is load-bearing and nothing said so — the previous arrangement reads as deliberate, with the assignment grouped next to the other module-level setup. The comment records that logging.py reads the variable at import time, so the next person tidying the imports does not undo it.

Not in scope

This does not touch the remaining nitpicky warnings. For context on why the raw count is not a defect count: of 1164 total warnings on a nitpicky build, 342 are aenum targets that are unresolvable by constructionconf.py:92-105 already documents why aenum is excluded from intersphinx_mapping, since its objects.inv carries zero py: objects — and a further 43 are pcap/pcapfile targets for optional dependencies not installed locally. The genuinely broken repository references number 22 and are tracked separately.

`pcapkit/utilities/logging.py:73` computes `SPHINX_TYPE_CHECKING` from
`os.environ` at *import* time, and `conf.py` imported `pcapkit` six lines before
setting the variable. Since importing `pcapkit` already pulls in the schema
modules, every `if SPHINX_TYPE_CHECKING:` TypedDict was left undefined and
autodoc could not document it.

- move the assignment above `import pcapkit`, with a comment saying why the
  order is load-bearing

Measured both orderings against `pcapkit.protocols.schema.application.httpv2`,
whose `FrameFlags` sits under that guard at line 36:

    set after import  -> SPHINX_TYPE_CHECKING = False, has FrameFlags = False
    set before import -> SPHINX_TYPE_CHECKING = True,  has FrameFlags = True

This clears 96 `autodoc: failed to import` warnings and restores 89 documented
members to the built HTML. All 89 names exist; none were stale.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review: GOOD TO GO

Reviewed at head 4fc191c4f on a different model, briefed to falsify rather than confirm, and to recommend closing this PR if the circular-import concern held. It does not hold.

The circular-import objection is falsified, four independent ways

This was the reason to doubt the change — that the post-import placement of os.environ['PCAPKIT_SPHINX'] = '1' might be deliberate.

  1. Direct experiment. Setting the variable before import pcapkit produces no ImportError, no partial-initialisation error, nothing.
  2. The decisive structural fact: all 14 SPHINX_TYPE_CHECKING-guarded blocks import only typing_extensionsTypedDict, sometimes Literal. None imports anything from pcapkit itself. So flipping the flag earlier cannot open a new import edge back into pcapkit's still-initialising module graph; there is nothing in those blocks that could recurse.
  3. A full real Sphinx build with the PR's own conf.py ran end to end, import pcapkit completing with only the expected optional-dependency warnings (pcap, pcapfile absent) and no traceback.
  4. The reload hooks re-read from source. Both process_docstring/remove_module_docstring and the raw module-reload block are commented out, and all of them set typing.TYPE_CHECKING — the ordinary forward-reference guard — never PCAPKIT_SPHINX or SPHINX_TYPE_CHECKING. The one live hook, bind_type_checking_names, calls sphinx_autodoc_typehints' guarded-import resolver, an unrelated mechanism. So nothing, live or dead, compensated for the original placement.

Confirmed

  • failed to import: 96 before, 0 after. Exact, by grep -c on both build logs.
  • 14 modules carry the guard, containing 89 TypedDicts — not 87. The undercount came from a class-only regex: ipv6.py uses the functional X = TypedDict(...) form exclusively. Largest contributors are mh.py (30) and tcp.py (15).
  • The FrameFlags probe verified at the HTML level, not just via import: in the baseline, HTTP.flags' annotation renders FrameFlags as bare unlinked text with no class page anywhere; after the change it is a working hyperlink to a real anchored definition that does not exist in the baseline output at all.

Two things this PR's description gets wrong, and I am correcting them

1. The warning totals are not reproducible. The description cites 1164 → 1057 with ~342 aenum-attributable. A plain sphinx-build -b html in this environment measures 156 → 83, with zero aenum matches. Two independent reviewers now measure ~156, so the 1164/1057 figures evidently came from a different invocation — most likely nitpicky -n mode, which would explain a much larger count from unresolvable cross-references. The direction is right and the 96 → 0 figure is exact; the absolute totals should not be quoted as they stand.

2. It is not "0 new problems". The reviewer reconciled its 156 → 83 delta exactly by category: 109 resolved (96 failed to import + 11 "more than one target found" + 2 "cannot resolve forward reference") against 36 newly introduced (33 duplicate object description plus 3 expanded "more than one target found"). Net 73 fewer, which matches 156 → 83 precisely.

The 36 new ones are worth understanding rather than ignoring: making these TypedDicts visible to autodoc for the first time means they now collide by bare name with pre-existing identically-named classes elsewhere — DFFFlags, MPLFlags and RPLFlags across hopopt.py and ipv6_opts.py, and Flags in tcp.py. That is a pre-existing naming ambiguity being surfaced, not something this fix does wrong, and the build still succeeds with exit 0. But it is a real trade and the description implied a clean sweep.

@JarryShaw — the naming collision is the one judgement call here. Net 73 fewer warnings and 96 previously-undocumented TypedDicts now rendering is clearly the better state, but if you would rather the duplicates were disambiguated first (via :noindex: or distinct names), that is a separate change and this PR can wait on it.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE, one judgement call for you — fixes 96 autodoc import failures (96 → 0) but surfaces 33 pre-existing duplicate object description warnings for DFFFlags/MPLFlags/RPLFlags/Flags, net 73 fewer overall; merge as-is, or disambiguate those names first as a separate change.

@JarryShaw
JarryShaw merged commit d596978 into main Sep 19, 2026
24 checks passed
JarryShaw added a commit that referenced this pull request Sep 20, 2026
#520 made the `if SPHINX_TYPE_CHECKING:` TypedDicts importable during the
build, which exposed that four of them are documented twice on the same page
-- once under "Header Schemas" beside the option they belong to, and again in
the "Type Stubs" appendix. Before #520 both occurrences failed to import, so
nothing collided.

- add `:no-index:` to the appendix copy of `RPLFlags`, `MPLFlags` and
  `DFFFlags` in `hopopt.rst` and `ipv6_opts.rst`, and of `Flags` in `tcp.rst`,
  keeping the inline "Header Schemas" occurrence canonical

Measured with sphinx-build 9.1.0: 83 warnings -> 50, `duplicate object
description` 36 -> 3, `failed to import` stays 0, no new warning appears. The
3 remaining duplicates pre-date #520 and are untouched. Also repairs cross
references, which resolved to the appendix via `#id0` rather than to the
canonical name anchor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant