Skip to content

fix(dumpkit): tree output raises on complex and is undocumented as lossy (#1266) - #1366

Merged
JarryShaw merged 1 commit into
mainfrom
fix/tree-lossy-1266
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/tree-lossy-1266

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (ran pylint, mypy and isort on the touched files; no new findings against main)
  • make test passes, and a test case covers the change (ran tests/project, util/run_unittest_leg.py dumpkit and the new module; not the full suite)
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible (N/A, added centrally)

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1266. Option (1), as ruled.

  • complex: make_dumper() overrides Tree._append_number. It writes a complex value as -> (1+2j), spelling NaN/Infinity the way the writer does for a float. Before, {'c': 1+2j} raised TypeError: must be real number, not complex (dictdumper/tree.py:397). The override applies only to Tree. JSON and PLIST are byte-identical to main over all 23 fixture captures (69 json/plist/tree reports, 27.8 MB, diff -r clean).
  • Docs: docs/source/pcapkit/dumpkit/index.rst now says tree is a lossy, human-readable view and names json/plist as the round-trip formats. A grep of docs/ and pcapkit/ found no claim that every dumper round-trips, so no other file changed. Since fix(dumpkit)!: output writers drop or corrupt what the packets carried #1305, NO_VALUE/NULL render as <NO_VALUE>/<NULL>, not NIL, so the note lists only '', b'', None and {}.
  • Tests: tests/dumpkit/test_tree_lossy_unit.py: 7 passed. With git show origin/main:pcapkit/dumpkit/common.py written over the fix: 4 failed (the four complex tree tests), 3 passed (JSON/PLIST and real-number pins, unchanged by design). tests/project: 389 passed, 1 skipped. run_unittest_leg.py dumpkit: 223 tests, 0 failures.

…ssy (#1266)

- make_dumper() overrides Tree._append_number to write a complex value as
  "-> (1+2j)", spelling NaN/Infinity as the writer does for a float;
  upstream math.isnan() raised TypeError on it. JSON and PLIST unchanged.
- docs/source/pcapkit/dumpkit/index.rst: state that tree is a lossy,
  human-readable view and that json and plist are the round-trip formats.
- tests/dumpkit/test_tree_lossy_unit.py pins both.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) docs Pull requests that change documentation only (docs: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 1614f7027: GOOD TO GO (Sonnet reviewed; author Opus).

  • Repro and fix: every complex case raises TypeError: must be real number, not complex on main, whether top-level, nested, in a list, inf/nan parts, 0j, -0j or 1e20+1e-20j. Here they render in the tree's own -> <number> style, e.g. c -> (1+2j) and (Infinity+NaNj).
  • Tree only: dictdumper defines _append_number only on JSON (json.py:311) and Tree (tree.py:389). PLIST inherits from XML, which has none. Tree derives straight from Dumper, so issubclass(output, dictdumper.tree.Tree) cannot match any other writer.
  • No change elsewhere: int, float, bool, nan, ±inf, 10**40, strings and None render byte-identically to main. All 23 fixtures dumped as json, plist and tree, PR versus main, are diff -r clean (69 outputs).
  • The docs note is accurate: '', b'', None and {} all render as NIL, a newline is written verbatim, and text/txt/tree map to dictdumper.Tree (extraction.py:225-227). No doc anywhere promises that every dumper round-trips.
  • Tests: the revert proof gives 4 failed / 3 passed. The dumpkit leg runs 223 tests with 0 failures, and tests/project gives 389 passed.

Nits: [] also collapses to NIL and could join the list in the note. The list-case test only asserts '-> 1' for the integer.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 85.36% (unit tier, Python 3.14, 1614f7027, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18777 1039 2348 847 90.21%
pcapkit/corekit 1930 137 602 45 91.47%
pcapkit/dumpkit 198 4 72 2 97.78%
pcapkit/foundation 2442 321 852 98 83.82%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15984 987 4164 564 91.28%
pcapkit/toolkit 495 159 150 17 62.48%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit a251f67 into main Oct 8, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/tree-lossy-1266 branch October 8, 2026 00:45
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

docs(dumpkit): tree output is lossy -- decide whether to document it or fix the writer

1 participant