docs: prune the Help Wanted page, fix the uninstallable docs extra, quiet 5 Sphinx warnings - #408
Merged
Merged
Conversation
…warnings **`pep.rst` was soliciting help for work already done**, which is worse than saying nothing to a would-be contributor. Every claim was checked against the code before editing, not taken from a list. Removed the solicitations for SCTP (all 13 RFC 9260 chunk types, 8 parameters, 13 error causes, real CRC32c, PPID dispatch), the two new engines, the logging integration and the test suite -- all verifiably present. Narrowed rather than deleted where the work is only partly done, which is more useful than a stale "help wanted": * ESP -- the four missing algorithms confirmed genuinely absent, and sharpened with the real numbers: 5 encryption and 5 integrity algorithms are applied. * Mobility Header -- FMIPv6 *has* landed, so the vague "requires some help" became what actually remains: 10 of 24 message data types, 51 of 71 options, 3 of 4 CGA extensions. * SCTP -- narrowed to the registered-but-unimplemented surplus: 17 of 30 chunk types, 24 of 32 parameters, 10 of 23 error causes. * The `NotImplemented` list stays, with two corrections: NDP's stub is under `link/`, not `internet/`, and QUIC was dropped because no stub exists. Three places where the page was simply wrong: it claimed `BaseWarning` still calls `warnings.simplefilter` (removed in `487d3da82`, with tests); it said 84 test modules where there are 86; and it claimed the suite is "bundled with the distribution" when `[tool.setuptools.packages.find]` excludes `test*`, so an installed PyPCAPKit has no `tests` package -- now recorded as a still-wanted item rather than a boast. The page also claimed to mirror discussion 106 while carrying 4 of its 6 comments; the missing two are added. The `pcap-ct` paragraph said "a way out that has not been adopted yet". It has been adopted -- as its own `PCAP_CT` engine in #405 -- so it now describes what shipped, why a separate engine rather than a second backend (both distributions own the import name `pcap`, and with both installed pcap-ct wins while upstream is shadowed), and the two caveats that remain: both are betas, and a runtime `libpcap.so.1` is still required because the `libpcap` wheel's published config sets `LIBPCAP = None`. **The `[docs]` extra could not install at all.** It named `sphinx-opengraph`, which 404s on PyPI; the real package is `sphinxext-opengraph`, which is what `docs/source/conf.py:61` imports. `sphinxcontrib-mermaid` was missing entirely though `conf.py:65` loads it. So `pip install -e '.[docs]'` failed outright, which means nobody could build the docs from a clean checkout. Both fixed and verified to resolve. **Sphinx warnings 268 -> 263.** The TOC suspicion did not pan out, and that is worth recording: across 128 pages there are 0 orphans, 0 broken toctree entries, 0 `automodule` targets pointing at nothing, and 0 unresolvable `:doc:` targets. All 19 newly added modules resolve to a reachable page, and the const-enumeration convention is correctly followed -- `esp.rst` and `sctp.rst` live under `docs/source/pcapkit/const/`, and the ESP protocol page only cross-references them. What was fixed: `sphinx.ext.autodoc.typehints` removed from `extensions` (an internal submodule with no `setup()`, so it only ever warned; the real work is done by the third-party `sphinx_autodoc_typehints`), and the duplicate-object warnings on `esp.rst`/`context.rst`. Those two were the only pages in the protocols/corekit tree using `automodule`, and the cause needed two attempts: `:no-index:` alone removed only half, because the *module docstring itself* carries `.. module::`, as 362 pcapkit modules do. Dropping the redundant `.. module::` from the `.rst` and keeping `:no-index:` leaves the docstring as the single declaration. The 263 remaining are pre-existing and none are in the new modules: 158 ambiguous cross-references (short names like ``:class:`TransType``` matching both `const.*` and `vendor.*`), ~40 `TYPE_CHECKING`-only forward references, 38 duplicate object descriptions on established pages, 22 guarded-import failures that are environment-only, and 5 autodoc signature crashes on `__protocol_type__`/`__schema__`. Left alone deliberately: those pages render correctly today and fixing them means judgement calls about rendered output.
There was a problem hiding this comment.
🟢 Approval recommended
Changes are limited to docs/dependency fixes and the updated dependencies align with the actual Sphinx extensions configured in docs/source/conf.py.
Pull request overview
This PR fixes the documentation build/install experience and updates the “Help Wanted” page to reflect current project status, while addressing a small set of Sphinx warnings originating in this repo.
Changes:
- Fix the
.[docs]optional dependency set by replacing the missingsphinx-opengraphpackage and adding the missing Mermaid dependency. - Prune and correct
docs/source/pep.rstso it no longer solicits work that has already landed and better reflects current gaps. - Reduce Sphinx warnings by removing an invalid extension entry and avoiding duplicate module declarations in two
automodulepages.
File summaries
| File | Description |
|---|---|
| pyproject.toml | Fixes docs extra dependency resolution (opengraph + mermaid). |
| docs/source/pep.rst | Updates “Help Wanted” content to match current implementation status and remaining gaps. |
| docs/source/pcapkit/protocols/internet/esp.rst | Avoids duplicate Sphinx object registration by removing redundant .. module:: and using :no-index:. |
| docs/source/pcapkit/corekit/context.rst | Same duplicate-registration mitigation pattern as esp.rst. |
| docs/source/conf.py | Removes sphinx.ext.autodoc.typehints from extensions to eliminate a warning and rely on sphinx_autodoc_typehints. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot's review on #408 was right. The intro said proposals are recorded "here in the discussion thread" and then "maintained here", conflating the page with the thread -- and once this branch added an ``important`` block that carefully distinguishes the two, the paragraph directly contradicted the note three lines above it. Reworded so "the discussion thread" and "this page" are always distinct: the proposals were *raised* in the thread, and the page is where they are *kept up to date*. Also fixed the tense, since 16k lines and 800 commits are long past, and pointed the questions sentence at real links rather than a bare "this thread" that had no referent on a rendered page.
Owner
Author
|
Fixed in The intro said proposals are recorded "here in the discussion thread" and then "documented and maintained here", conflating the page with the thread. And because this branch added an Reworded so "the discussion thread" and "this page" are always distinct: the proposals were raised in the thread; the page is where they are kept up to date. Two things fixed while there:
|
…odule These two were the **only two of 128 pages** using `automodule`; every other page declares `.. module::` and then lists its members explicitly. Converted to match: the descriptive prose is carried in the `.rst`, and each class, function and data item gets its own entry. That also removes the `:no-index:` workaround this branch had added, for a better reason than suppressing a symptom -- the duplicate-object warnings were *created by* `automodule`, not by two `.. module::` directives coexisting. Which answers the question the workaround raised. 365 modules carry `.. module::` in their docstring and 122 of 128 pages carry one too, and that is not a duplication: **without `automodule`, Sphinx never parses the module docstring at all**, so the docstring's directive is inert text in a `.py` file and the page's is the only registration. `automodule` was what made them collide. Two traps the conversion had to handle, either of which would have silently degraded the page: * `esp.py`'s docstring defines the `|cryptography|` substitution and its link target, and **eight rendered member docstrings reference them** -- `load_cryptography`, `_CRYPTO`, `CipherSuite.requires_cryptography`, `ESPStatus.UNSUPPORTED`, `SecurityAssociation.decrypt`/`encrypt`. `automodule` had been supplying those definitions invisibly; dropping it without carrying them into the page would have broken every one. * Both pages were rendering their title **twice**, because `automodule` emitted the docstring's own title as a second heading. Now gone. Verified rather than assumed. Full builds before and after: 267 total, 263 warning lines, and a `diff` of the sorted warning sets is byte-for-byte identical -- zero new, zero removed. Neither page had a warning attributed to it either way. Member coverage checked by grepping every `id="pcapkit…"` out of the built HTML: `context.html` 13 -> 14, `esp.html` 74 -> 75, **nothing lost**; the additions are `_CT` and `_CRYPTO`, which `automodule` had skipped. The rendered visible text was diffed too: every sentence, table row and footnote survives.
…a decision Two corrections to the Test Cases section, both from measurement. The module count said 86; `find tests -name 'test_*.py'` counts **91**. The number has been wrong twice now (84 before this branch, 86 after), which is an argument for not quoting it at all -- but it is useful context for a contributor, so it is corrected rather than dropped. "Shipping the suite ... is not done" framed a deliberate choice as an omission. Built both artefacts to settle it: the **sdist carries all 91 modules**, the **wheel carries none**. So a distribution packager building from source has them, and `pip install pypcapkit` does not. Keeping the wheel lean is the right call, and the reason is stronger than "wheels don't usually ship tests": the suite *could not run* from an installed package anyway. The generated sample captures are not shipped, and `tests/_tiers.py` resolves paths from a repository root that an installed package does not have. Anyone who wants to run the tests wants the repository. Reworded to say that, rather than leaving a standing invitation to "fix" it by stuffing tests into the wheel. That left a single remaining item under a "Two things are still wanted" heading, so the list became a sentence.
This was referenced Sep 16, 2026
JarryShaw
added a commit
that referenced
this pull request
Sep 16, 2026
…rate (#417) * foundation: recover a failed next layer through the protocol, not the schema `beholder` read the fallback payload from `self.__header__.get_payload()`. That is what `ProtocolBase._get_payload()` wraps, so the two agree for every protocol whose payload is a schema field -- and disagree for the two that override it. SCTP carries user data inside a DATA chunk and PCAP-NG inside a block, so neither header schema has a `payload` field at all, and reaching for the schema raises `ProtocolUnbound("unknown field: 'payload'")` *from the recovery path*. A next-layer parse failure that should have degraded to `Raw` became a crash that aborted the frame. It was unreachable until now only because `SCTP.__proto__` had no entries: with nothing registered on a payload protocol identifier, no next-layer parse could fail. Registering anything on one exposes it immediately. - `beholder` now calls `self._get_payload()`, the wrapper both overrides. - It also forwards `alias=proto`, which the success path in `_import_next_layer` already passes. Without it, a payload that failed to parse came back as a bare `Raw` while an *unregistered* number came back named -- so registering a protocol made the output less informative than leaving the number alone. A plain integer has no `name` and still renders as `Raw`, so this only adds a name where the registry key is an enumeration. The four `beholder` tests are rewritten onto shared stand-ins that carry a `_get_payload`, as a real protocol does, plus two new ones: the alias forwarding and an overridden `_get_payload` whose schema accessor raises, which is the SCTP shape. Unit tier green, and mypy reports the same five pre-existing errors in `decorators.py` as it does at HEAD -- no new ones. * protocols: implement NGAP over SCTP, decoding aligned PER through pycrate (#251) NGAP (3GPP TS 38.413) is the 5G RAN-to-AMF control plane. It has no header of its own: an SCTP DATA chunk whose payload protocol identifier names NGAP carries exactly one aligned-PER `NGAP-PDU` and nothing else, so nothing about it is visible until an ASN.1 decoder has run. - `pcapkit.protocols.application.ngap`, with the matching schema and data models, and exports added everywhere `FTP` is listed. - Registered on `SCTP.__proto__` as a *default*, for PPID 60 (`NG_Application_Protocol`) and 66 (`NGAP_over_DTLS_over_SCTP`), following how `TCP.__proto__` declares 21 and 80 rather than calling `register_sctp` at import time from somewhere. - `pycrate` is an optional extra, `pip install pypcapkit[NGAP]`, imported inside the parse path and never at module import, and deliberately excluded from `all` -- it lands ~238 MB of `pycrate_asn1dir` (87 spec modules) to obtain the one 4.9 MB `NGAP.py`, and it is LGPL-2.1+ where this package is BSD-3-Clause. Absent, it fails the way `pcapkit.protocols.internet.esp` fails without `cryptography`: a `ProtocolError` that `beholder` degrades to `Raw`. - Decoding is generic rather than per-procedure: the value tree is mapped by ASN.1 *shape*, so all 81 elementary procedures and 438 protocol IEs work without 519 hand-written cases and a new 3GPP release needs no code change. The PDU kind, procedure code, criticality, message type name and IE list are surfaced as first-class fields. - `ProcedureCode`, `Criticality` and `ProtocolIE` live in `ngap.py`, not `pcapkit.const`: 3GPP publishes them in the ASN.1 of TS 38.413 rather than in a crawlable registry, so there is no vendor module either. The SCTP PPIDs are IANA's and are used from `pcapkit.const.sctp` as they stand. `reset_val()` is never called on the parse path, and there is a comment plus a test saying so: it walks all 318 submodules of the compiled specification and costs ~102 ms against the decode's ~0.15 ms, and `from_aper()` overwrites the value anyway. The module-level PDU object is stateful and `get_val()` hands back its own containers, so a lock is held across the decode *and* the conversion. Five SCTP tests used PPID 60 as an arbitrary placeholder with junk payloads and now describe the registered default instead; their `SCTP.__proto__.clear()` teardowns are replaced with a snapshot restore, since clearing the registry now destroys the defaults for every later test in the process. Unit tier: 670 passed with pycrate, 613 passed / 65 skipped without it, no failures either way. Round trip verified byte-exact on a real 58-octet `NGSetupRequest`. * docs: add the NGAP protocol page (#251) Follows the convention the sibling application pages use: the module docstring copied into the `.rst` under a `.. module::` directive, then `autoclass` per class, rather than `automodule` -- which was the review feedback on #408. `ProcedureCode` and `ProtocolIE` are rendered with `:no-members:`. Between them they carry 519 members, each named for the 3GPP identifier it came from, and a page listing all of them is longer than the specification's own tables and no more useful; a note says where to read them instead, and why there is no matching `pcapkit.vendor` module. No `pycrate` intersphinx mapping: the project publishes no `objects.inv`, so the link is a plain external reference through the same `|pycrate|_` substitution `esp.rst` uses for `cryptography`. One line added to the application-layer toctree. Sphinx build verified against a clean export of HEAD with the same interpreter: 285 warnings before, 285 after -- none added, none removed. * tests: update the Raw fallback assertion the beholder alias fix invalidates CI's Integration leg failed on all six interpreters with one real failure: `test_malformed_http_payload_falls_back_to_raw` asserted `tcp.payload.info.protocol is None`, and forwarding `alias` from `beholder` makes it 80. Neither the unit tier nor the local runs cover that tier, which is how it reached CI. The new value is the right one -- `Data_Raw.protocol` is "the original enumeration of this protocol", and 80 is what the payload arrived as -- so the assertion is updated rather than the fix reverted. The protochain still reads `Raw`, since a plain int has no name to render. The comment justifying the forward was too broad, though, and is now measured rather than asserted. It claimed an unregistered code keeps its name generally. True on SCTP (an unknown PPID gives `SCTP:Unassigned_4243` and `protocol=4243`, which is why registering NGAP would otherwise have made a failed parse *less* informative than leaving PPID 60 alone) and on IP, but false on TCP, where `Transport._decode_next_layer` resolves ports through `__proto__` and reaches Raw without an alias, so an unknown port already reports `None`. So the failure path is now uniform across layers while the unknown-code paths remain inconsistent with each other -- filed as #418. Full suite, the command CI's Integration leg runs: 806 passed, 17 skipped. * ngap: correct the procedure count, and say that PPID 66 does not decode Four review findings on #417, all of them right. The "not 51 hand-written procedures" heading in `ngap.py` and `ngap.rst` was wrong: `ProcedureCode` has 81 members, and 51 was a bad number from the brief that the enums themselves already corrected. Now 81 in both, which is the same character count, so the RST underline is unaffected. The other two are accurate-but-misleading-by-omission. `SCTP`'s class docstring said both PPIDs are registered to NGAP without saying that only 60 can decode: a PPID 66 payload is an NGAP PDU inside a DTLS record, and with no DTLS implementation those bytes are never aligned PER, so 66 degrades to `Raw` on every capture rather than only when `pycrate` is absent. The module docstring and the docs page already said so; the class docstring, which is where someone reading `SCTP.__proto__` looks, did not. `test_ppid_dispatch_hook`'s docstring had the same gap, and its assertion is deliberately no stronger than "both dispatch" -- now stated, so the test is not read as claiming both decode. 56 tests pass across the SCTP and NGAP files. * ngap: drop Criticality.get's unused default, and report one error type Copilot is right on #417: `Criticality.get` declared and documented a `default` it never used. An unknown name raised `KeyError`, an unknown value `ValueError` via `_missing_`, and `default` reached neither path. The behaviour is intended -- `Criticality` is an ASN.1 `ENUMERATED` with no extension marker, so a fourth value is unencodable and a lookup for one is a bug, which is why `_missing_` raises deliberately. It is the parameter that was wrong, not the closedness, so the parameter is gone. No caller passed it: all five call sites use one argument. Its siblings keep theirs, and the docstring now says why rather than leaving the difference to be rediscovered -- `ProcedureCode` and `ProtocolIE` extend through `extend_enum` for a code a newer specification names, which is right for a registry that grows and impossible for one that cannot. Also made the unknown-name path raise `ValueError` like the unknown-value path, so the two ways of getting this wrong no longer report differently. Verified: signature carries no `default`, valid int/str/member lookups unchanged, both unknown forms now `ValueError`. 22 NGAP tests and the SCTP file pass.
JarryShaw
added
the
docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three things: the
[docs]extra could not install at all,pep.rstwas asking for help with work already finished, and a handful of Sphinx warnings were ours rather than pre-existing.The
[docs]extra was uninstallablepip install -e '.[docs]'failed outright, so nobody could build the documentation from a clean checkout:sphinx-opengraph404s on PyPI. The real package issphinxext-opengraph, which is whatdocs/source/conf.py:61imports.sphinxcontrib-mermaidwas missing from the extra entirely, thoughconf.py:65loads it.Both fixed; the extra now resolves (verified with
pip install --dry-run, exit 0).pep.rstwas soliciting help for finished workWhich is worse than saying nothing — a contributor reads it and starts on something already shipped. Every claim was checked against the code before editing.
Solicitations removed, each verified present: SCTP (all 13 RFC 9260 chunk types, 8 parameters, 13 error causes, real CRC32c, PPID dispatch), both new engines, the logging integration, the test suite.
Narrowed rather than deleted, which is more useful than a stale "help wanted":
NotImplementedlistlink/(notinternet/) and QUIC dropped — no stub existsThree places the page was simply wrong:
BaseWarningstill callswarnings.simplefilter. That was removed in487d3da82, with tests. Deleted; thewarn()double-reporting item is still true and stays.[tool.setuptools.packages.find]excludestest*, so an installed PyPCAPKit has notestspackage. Now recorded as a still-wanted item rather than a boast.The page also claimed to mirror discussion 106 while carrying 4 of its 6 comments; the missing two are added.
The
pcap-ctparagraph said "a way out that has not been adopted yet". It has been adopted — as its ownPCAP_CTengine in #405. It now describes what shipped, why a separate engine rather than a second backend (both distributions own the import namepcap; with both installed pcap-ct wins and upstream is shadowed), and the two caveats that remain: both are betas, and a runtimelibpcap.so.1is still required because thelibpcapwheel's config setsLIBPCAP = None.Sphinx warnings: 268 → 263
The TOC suspicion did not pan out, and that is worth recording so nobody re-investigates it. Across 128 pages: 0 orphans, 0 broken toctree entries, 0
automoduletargets pointing at nothing, 0 unresolvable:doc:targets, 0 pages with multiple toctree parents. All 19 newly added modules resolve to a reachable page, and the const-enumeration convention is correctly followed —esp.rstandsctp.rstlive underdocs/source/pcapkit/const/, and the ESP protocol page only cross-references them rather thanautomodule-ing them.What was actually fixed:
sphinx.ext.autodoc.typehintsremoved fromextensions— an internal submodule with nosetup(), so it only ever produced a warning. The real work is done by the third-partysphinx_autodoc_typehints.esp.rstandcontext.rst, the only two pages in the protocols/corekit tree usingautomodule. The cause needed two attempts::no-index:alone removed only half of them, because the module docstring itself carries.. module::— as 362 pcapkit modules do. Dropping the redundant.. module::from the.rstand keeping:no-index:leaves the docstring as the single declaration. Both files carry a comment explaining why they deviate.The 263 remaining are pre-existing and none are in the new modules, itemised so the next person doesn't have to re-count:
TransType``` matching bothconst.` and `vendor.`TYPE_CHECKING-only names in annotationsipv6_opts,hopopt,tcp,mh, …), 0 involving esp/sctp/context.[all]__protocol_type__/__schema__Left alone deliberately: those pages render correctly today, and fixing the duplicates means judgement calls about rendered output on pages this PR has no other reason to touch.
Not done here
The 158 ambiguous cross-references and ~40 forward references need docstring changes across
pcapkit/, which is a separate sweep. Same for the 5 autodoc crashes.