Repository navigation
docs: fix Extractor's exception mismatch and 40 phantom/stale Args: labels - #501
Merged
Merged
Conversation
- Extractor.__call__ documented IterableError but raises CallableError
(a sibling exception class, not a subclass); fixed the docstring to
name the class actually raised, since the message and semantics
("is not callable") back CallableError.
- Fixed six Args: blocks documenting phantom parameters left over from
copy-paste: IPv6_Route._read_data_type_rpl (length -> schema/header),
PCAPNG._make_option_if_hardware/_make_option_isb_starttime/
_make_option_isb_endtime/_make_secrets_wireguard, and
OSPF._make_encrypt_auth (phantom auth_type removed).
- Renamed ~34 stale option: labels to options: across every
_read_opt_*, make(), and _make_hopopt_options/_make_ipv6_opts in
hopopt.py and ipv6_opts.py, matching each method's real parameter.
- Also fixed, found via the same signature sweep: a "timestmap" typo
in three PCAPNG block-maker docstrings and one summary line, and a
phantom `options` param on PCAPNG._make_block_systemd.
- Added a test pinning Extractor(auto=True)() to raise CallableError
(and __iter__ to IterableError) against a real constructed instance,
so the raise and the docstring can't drift apart unnoticed again.
Verified every touched method's Args: block against inspect.signature
with a standalone script; tests/foundation, test_ipv6_extension_unit
and test_pcapng_unit all pass.
…490) The prose read as though RFC 815 itself uses the RCVBT bit table, when RFC 815 actually presents hole descriptors as an alternative to the RCVBT approach used by RFC 791. The eight algorithm steps were already accurate; this only fixes the historical framing sentence.
This was referenced Sep 19, 2026
JarryShaw
added a commit
that referenced
this pull request
Sep 20, 2026
…#519) Closes #519. #501 fixed one wrong exception name and forty stale `Args:` labels by hand. #519 showed the class was not exhausted. Re-deriving its census turned up one finding that is not a docstring defect at all, plus a third class the issue never mentions. `TCP._read_mode_sack` documented `ProtocolError: If length is **NOT** multiply of 8 plus 2` and never checked it. The tempting reading -- and the first one taken here -- is that the clause was stale, like the other five phantoms. It is the opposite, and three things settle it: RFC 2018 gives SACK a 2-octet header followed by 8-octet edge pairs; both neighbouring readers validate their own lengths, `_read_mode_sackpmt` on `!= 2` and `_read_mode_echo` on `!= 6`, with the same message; and nothing upstream enforced it either, since the schema's `ListField` consumes as many whole 8-octet items as it finds and ignores the tail. So the docstring was right and the code was wrong. - tcp.py: add the `(length - 2) % 8` check to `_read_mode_sack`, raising `ProtocolError` with the message its two siblings already use. - httpv2.py: drop the `Raises: ProtocolError` clause from the five `_read_http_*` readers that cannot raise it. `_read_http_none` shows the mechanism -- its `raise` is commented out and replaced by a `ProtocolWarning` on the next line. All 11 `_read_http_*` methods carried the clause; the 6 that genuinely raise keep it. - tcp.py, twice: un-indent a `Returns:` header that sat inside the `Args:` block. This is the class the issue missed, and it loses documentation rather than merely misnaming it -- napoleon parses the header as a parameter, so the page rendered `:param Returns:` and carried no `:returns:` at all. Measured through `GoogleDocstring` directly, before and after. - frame.py, schema/application/httpv2.py, schema/misc/pcapng.py: correct four `Args:` labels naming a parameter that does not exist. None of the four takes `**kwargs`, so each is a hard `TypeError`, not a cosmetic slip: `Frame.register(code=..., module=IPv4)` really does raise `TypeError: got an unexpected keyword argument 'module'`, while `protocol=` is accepted. - hip.py: document the parameters `_make_param_reg_response` and `_make_param_route_dst` omit, in the wording their own siblings `_make_param_reg_failed` and `_make_param_route_via` already use. - tests: `test_tcp_sack_length_unit.py` pins the length rule, accepted and rejected cases both. `test_docstring_contract.py` walks every function under `pcapkit/` and checks each documented name against the real signature, each documented exception against the real `raise` statements, and each section header against its indentation. It imports no pcapkit, reading source under its own root, so no editable install can shadow what it measures. Census re-derived at this base rather than taken from the issue, which counted against an older one: phantom Raises: 6 -> 0 remaining Args: naming a missing parameter 13 -> 9 remaining, all allowlisted swallowed section headers 3 -> 1 remaining, allowlisted The `Args:` figure is 13 here and was 14 when #519 was written: #511 retired the IPX socket scrape and renamed that crawler's `soup` parameter to `data`, incidentally fixing one. The nine that remain are in files owned by other in-flight changes and are recorded in `KNOWN_DEFECTS` with a reason each, under a test asserting every one still reproduces -- so the list cannot rot into a description of bugs that are gone. That test has already earned its keep: it failed on exactly the IPX entry during the rebase, which is how the stale entry was found and removed. Be clear about what is provable. A docstring label is not executed, so the corrections cannot fail a test individually -- the contract test is what pins them, by deriving the answer from the code instead of snapshotting today's wrongness. Verified by reintroducing each class into a scratch tree: baseline exit 0, phantom `Raises:` exit 1, bad `Args:` label exit 1, re-indented `Returns:` exit 1. The two `Raises:` assertions are complementary rather than redundant, and `_read_http_none` proves it -- `any(header.flags)` in its body makes the conservative reachability check judge it possibly-raising, so only the commented-out-raise check catches it. Measured: reachability exit 0, commented raise exit 1. The SACK change is the one with a real behavioural fails-before. Lengths 3, 11, 14, 17, 19 and 25 all parsed clean on `c8fd97bcd` with the SACK option present in `tcp.info['options']`, and all six now raise `ProtocolError: TCP: [OptNo 5] invalid format`. Two things deliberately not done. All 12 functions carrying both `**kwargs` and an `Args:` section without documenting `**kwargs` are in hip.py -- 438 of 450 document it elsewhere -- and that set includes the two siblings this change copies its wording from, so fixing 4 of the 12 is what would make hip.py inconsistent. And the SACK check implements exactly the documented rule, not RFC 2018's one-to-four block bound; `length=2` is degenerate and still parses, recorded in the test as a decision. Tests: 5 passed / 10 subtests (contract), 3 passed / 10 subtests (SACK), 25 passed / 19 subtests (tier guard), 81 passed / 73 subtests (transport and schema units, unchanged by the new check). An independent second scanner, written from scratch against the same baseline, reproduced all three counts (6 / 13 / 3) and the residuals (0 / 9 / 1), and turned up three things now recorded in the test module: - `_documented_names` justified its relative-indent measurement by saying `__doc__` is dedented at compile time. True of `__doc__` and irrelevant here, since this module reads `ast.get_docstring(..., clean=False)`, which preserves raw source indentation -- measured [0, 8, 12, 12] against [0, 0, 4, 4] for the dedented forms. The implementation was right for a different reason (nesting depth moves the absolute column); the rationale is corrected rather than left wrong in a checker for wrong rationales. - `_raised_names` resolves only `Name` and `Attribute` raise targets, and `ast.walk` attributes a nested `def`'s raise to the enclosing function. Both can only miss a phantom, never fail a correct docstring, and both are now documented with their measurements: 856 `Name` + 2 `Attribute` + 0 `Subscript` across 858 raise targets, and 11 documented functions with a nested `def`, 3 raising inside it. The second is the correct answer rather than a gap -- `_read_param_locator_set` documents `ProtocolError` and raises none itself, but calls a `_read_locator` helper that does. - One reported defect was a false positive and is recorded as a trap: `Raw.__post_init__` documents `error` and `alias`, has neither in its signature, and assigns a local `alias` -- but forwards `**kwargs` to `read`, which declares both as keyword-only and uses them. Reporting it would ask for correct documentation to be deleted.
JarryShaw
added a commit
that referenced
this pull request
Sep 20, 2026
…#519) (#535) Closes #519. #501 fixed one wrong exception name and forty stale `Args:` labels by hand. #519 showed the class was not exhausted. Re-deriving its census turned up one finding that is not a docstring defect at all, plus a third class the issue never mentions. `TCP._read_mode_sack` documented `ProtocolError: If length is **NOT** multiply of 8 plus 2` and never checked it. The tempting reading -- and the first one taken here -- is that the clause was stale, like the other five phantoms. It is the opposite, and three things settle it: RFC 2018 gives SACK a 2-octet header followed by 8-octet edge pairs; both neighbouring readers validate their own lengths, `_read_mode_sackpmt` on `!= 2` and `_read_mode_echo` on `!= 6`, with the same message; and nothing upstream enforced it either, since the schema's `ListField` consumes as many whole 8-octet items as it finds and ignores the tail. So the docstring was right and the code was wrong. - tcp.py: add the `(length - 2) % 8` check to `_read_mode_sack`, raising `ProtocolError` with the message its two siblings already use. - httpv2.py: drop the `Raises: ProtocolError` clause from the five `_read_http_*` readers that cannot raise it. `_read_http_none` shows the mechanism -- its `raise` is commented out and replaced by a `ProtocolWarning` on the next line. All 11 `_read_http_*` methods carried the clause; the 6 that genuinely raise keep it. - tcp.py, twice: un-indent a `Returns:` header that sat inside the `Args:` block. This is the class the issue missed, and it loses documentation rather than merely misnaming it -- napoleon parses the header as a parameter, so the page rendered `:param Returns:` and carried no `:returns:` at all. Measured through `GoogleDocstring` directly, before and after. - frame.py, schema/application/httpv2.py, schema/misc/pcapng.py: correct four `Args:` labels naming a parameter that does not exist. None of the four takes `**kwargs`, so each is a hard `TypeError`, not a cosmetic slip: `Frame.register(code=..., module=IPv4)` really does raise `TypeError: got an unexpected keyword argument 'module'`, while `protocol=` is accepted. - hip.py: document the parameters `_make_param_reg_response` and `_make_param_route_dst` omit, in the wording their own siblings `_make_param_reg_failed` and `_make_param_route_via` already use. - tests: `test_tcp_sack_length_unit.py` pins the length rule, accepted and rejected cases both. `test_docstring_contract.py` walks every function under `pcapkit/` and checks each documented name against the real signature, each documented exception against the real `raise` statements, and each section header against its indentation. It imports no pcapkit, reading source under its own root, so no editable install can shadow what it measures. Census re-derived at this base rather than taken from the issue, which counted against an older one: phantom Raises: 6 -> 0 remaining Args: naming a missing parameter 13 -> 9 remaining, all allowlisted swallowed section headers 3 -> 1 remaining, allowlisted The `Args:` figure is 13 here and was 14 when #519 was written: #511 retired the IPX socket scrape and renamed that crawler's `soup` parameter to `data`, incidentally fixing one. The nine that remain are in files owned by other in-flight changes and are recorded in `KNOWN_DEFECTS` with a reason each, under a test asserting every one still reproduces -- so the list cannot rot into a description of bugs that are gone. That test has already earned its keep: it failed on exactly the IPX entry during the rebase, which is how the stale entry was found and removed. Be clear about what is provable. A docstring label is not executed, so the corrections cannot fail a test individually -- the contract test is what pins them, by deriving the answer from the code instead of snapshotting today's wrongness. Verified by reintroducing each class into a scratch tree: baseline exit 0, phantom `Raises:` exit 1, bad `Args:` label exit 1, re-indented `Returns:` exit 1. The two `Raises:` assertions are complementary rather than redundant, and `_read_http_none` proves it -- `any(header.flags)` in its body makes the conservative reachability check judge it possibly-raising, so only the commented-out-raise check catches it. Measured: reachability exit 0, commented raise exit 1. The SACK change is the one with a real behavioural fails-before. Lengths 3, 11, 14, 17, 19 and 25 all parsed clean on `c8fd97bcd` with the SACK option present in `tcp.info['options']`, and all six now raise `ProtocolError: TCP: [OptNo 5] invalid format`. Two things deliberately not done. All 12 functions carrying both `**kwargs` and an `Args:` section without documenting `**kwargs` are in hip.py -- 438 of 450 document it elsewhere -- and that set includes the two siblings this change copies its wording from, so fixing 4 of the 12 is what would make hip.py inconsistent. And the SACK check implements exactly the documented rule, not RFC 2018's one-to-four block bound; `length=2` is degenerate and still parses, recorded in the test as a decision. Tests: 5 passed / 10 subtests (contract), 3 passed / 10 subtests (SACK), 25 passed / 19 subtests (tier guard), 81 passed / 73 subtests (transport and schema units, unchanged by the new check). An independent second scanner, written from scratch against the same baseline, reproduced all three counts (6 / 13 / 3) and the residuals (0 / 9 / 1), and turned up three things now recorded in the test module: - `_documented_names` justified its relative-indent measurement by saying `__doc__` is dedented at compile time. True of `__doc__` and irrelevant here, since this module reads `ast.get_docstring(..., clean=False)`, which preserves raw source indentation -- measured [0, 8, 12, 12] against [0, 0, 4, 4] for the dedented forms. The implementation was right for a different reason (nesting depth moves the absolute column); the rationale is corrected rather than left wrong in a checker for wrong rationales. - `_raised_names` resolves only `Name` and `Attribute` raise targets, and `ast.walk` attributes a nested `def`'s raise to the enclosing function. Both can only miss a phantom, never fail a correct docstring, and both are now documented with their measurements: 856 `Name` + 2 `Attribute` + 0 `Subscript` across 858 raise targets, and 11 documented functions with a nested `def`, 3 raising inside it. The second is the correct answer rather than a gap -- `_read_param_locator_set` documents `ProtocolError` and raises none itself, but calls a `_read_locator` helper that does. - One reported defect was a false positive and is recorded as a trap: `Raw.__post_init__` documents `error` and `alias`, has neither in its signature, and assigns a local `alias` -- but forwards `**kwargs` to `read`, which declares both as keyword-only and uses them. Reporting it would ask for correct documentation to be deleted.
JarryShaw
added
the
docs
JarryShaw
added a commit
that referenced
this pull request
Oct 3, 2026
…issue (#719) - Nine citations in tests/ called a pull request "GitHub issue" (#921, #764, #906, #815, #936, #721, #501, #983) or lumped PR #428 in with issue #425; each now names the right kind. - test_dispatch_default_resolution_unit: issue #425 reported the registry leak and PR #428 fixed it, so the sentence says "reported" and "fixed" instead of crediting the issue with the fix. - Prose only: docstrings and comments, no assertion or logic touched.
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
1 participant
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.
Fixes #490, except the two
ipv4.pyitems which ship in #499 — that file was owned by another concurrent stream.Defect 1 —
Extractor.__call__documented an exception it does not raiseextraction.py:1019-1023promisedIterableError;:1041raisesCallableError. These are siblings, bothBaseError, TypeError, and neither subclasses the other — measured, both directionsFalse— soexcept IterableError:written straight from the docstring did not catch it.The docstring was wrong, not the raise.
__call__failing is a callability problem, the message already says "is not callable", and theIterableErrortext was copied from the sibling__iter__three lines above, which raises it correctly under the identical condition. So the fix corrects the docstring and leaves the behaviour alone.Now consistent:
Defect 2 — six phantom
Args:parametersEach confirmed against
inspect.signaturebefore editing:IPv6_Route._read_data_type_rpl(length→schema/header),PCAPNG._make_option_if_hardware(os→hardware),_make_option_isb_starttimeand_make_option_isb_endtime(bothip→timestamp, byte-identical docstrings copied from the preceding_make_option_ns_dnsip6addr),_make_secrets_wireguard(data→entries), andOSPF._make_encrypt_auth(phantomauth_typeremoved; it takes onlyauth_data).Defect 3 — 34
option/optionslabel mismatches, not "roughly thirty"17 in
hopopt.pyand 17 inipv6_opts.py, verified by counting the removed lines in the diff. The issue estimated ~30 across 14_read_opt_*per file; the real figure is 15 per file plusmake()and_make_hopopt_options/_make_ipv6_opts. Zero phantomoption:labels remain in either file.Found by the sweep and also fixed — same class of bug, same files
timestmap→timestamptypos, inPCAPNG._make_block_epb,_make_block_isb,_make_block_packetand_read_timestamp's summary.options: Block options.inPCAPNG._make_block_systemd, which takes nooptionsparameter at all.Independently verified, not taken on trust
I re-ran my own signature checker across all six touched classes rather than relying on the agent's: 320 methods with
Args:blocks, 2 phantom parameters remaining — and both are the single case deliberately left alone below.Deliberately NOT fixed — wants its own issue
IPv6_Route.__post_init__documentssrc_ipanddst_ip, absent from the runtime signature (file, length, extension, **kwargs). This is not a copy-paste phantom like the others: an@overloadstub atipv6_route.py:353-354genuinely declares both as typed keyword parameters. Confirmed they appear nowhere else inpcapkit/ortests/— only in that overload and this docstring. It reads as dead or unfinished plumbing behind a type-checking overload, and correcting the docstring alone would put it in direct contradiction with the overload. Filing separately.Three further flags were false positives and correctly left alone:
PCAPNG.__index__'sselfis a deliberateOptional[PCAPNG] = Nonehack (the code says so) letting it work as both instance method and pseudo-classmethod, so documenting it is right; andPCAPNG.read's_read/_seek_setare documented as RST-escaped\_read:/\_seek_set:, which a naive regex misses.Also included
docs/source/pcapkit/foundation/reassembly/tcp.rstframed the algorithm as RFC 815 "dealing withRCVBT", whereas RFC 815 presents hole descriptors as an alternative to RFC 791's bit table. Reworded in its own commit, since it is unrelated to the code changes. This was raised in #490 for judgement rather than filed as a defect.Verification
Revert-proof. A docstring-only change cannot be proven by reverting the docstring, so I reverted the code instead — made
__call__raiseIterableError, the class the old docstring claimed:Restored:
tests/foundation/test_extraction.py12 passed, 28 subtests passed.Blast radius
test_ipv6_extension_unit.py+test_pcapng_unit.py: 89 passed, 217 subtests passed. Fulltests/foundation/: 214 passed, 11 skipped.The new test builds a real
Extractor(auto=True)rather than a stand-in, and assertsextractor()raisesCallableError,iter(extractor)raisesIterableError, and neither is a subclass of the other — so the docstring and the raise cannot drift apart again.No Sphinx build was run. The rendering claims rest on the
automethoddirectives still pointing at the same, now-corrected methods, not on built HTML.