Skip to content

fix(ipv4): build the TS option from its default timestamp, and print a bad one with !r (#2277) - #2347

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2277-ipv4-ts-default
Oct 27, 2026
Merged

fix(ipv4): build the TS option from its default timestamp, and print a bad one with !r (#2277)#2347
JarryShaw merged 1 commit into
mainfrom
fix/2277-ipv4-ts-default

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2277.

  • Fix: IPv4._make_opt_ts now treats an omitted or None timestamp as an empty list, giving a Timestamp-only option with no slots, instead of raising on its own default. Any other bad value raises with the house-form message and !r.
  • Fail-without-fix: 3 failed on origin/main. Passing on the rebased head.

@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at cd3ec38ac (Sonnet) — one stale comment, code and tests are sound.

  • Confirmed: omitted/None/[] give the same byte-identical default option across 8 variations each; 15 bad values all raise the exact house-form message; IPv4.from_data rebuilds byte-exact; the changed existing test is correctly retargeted.
  • Fail-without-fix: 3 failed on origin/main. On the head, the new file gives 2 passed, and the broader IPv4/round-trip/contract/tier selection gives 143+49+21+68 passed. mypy reports nothing new.
  • Required: examples/generators/options.py's TS override comment still says timestamp=None "is rejected outright," which this PR falsifies. Reword it; keep the override (dropping it changes the fixture).
  • Non-blocking: no versionchanged bullet (the docs(protocols): version directives and accurate prose for the internet layer (#1103) #1760-style sweep usually adds these later); a separate pre-existing defect (LSR/RR/SSR options overflow their field with default counts=10) is unrelated to this issue.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw
fix(ipv4): build the TS option from its default timestamp, and print …
5e27d30 
…a bad one with !r (#2277)

Problem: _make_opt_ts accepted only a list or a dict for timestamp, but its
default is None, so a TS option made without timestamps always raised
ProtocolError; the error printed the value without !r.

Fix: treat None as an empty list before the type dispatch, so it builds a
Timestamp-only option with `counts` empty slots; any other non-list,
non-dict raises "invalid timestamp value: must be a list or a dict, not
<repr>". The docstring's Raises names ProtocolError for exactly that.

Test: test_ipv4_ts_default_unit pins the default option make -> parse ->
rebuild byte for byte and the message text; test_ipv4_unit's
timestamp=None ProtocolError case now uses 'abc'.
@JarryShaw
JarryShaw force-pushed the fix/2277-ipv4-ts-default branch from cd3ec38 to 5e27d30 Compare October 27, 2026 02:09
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Amended cd3ec38ac → 5e27d3056: reworded the stale examples/generators/options.py comment, kept the override. test_ipv4_ts_default_unit.py, test_ipv4_unit.py and test_docstring_contract.py give 36 passed.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 27, 2026
@github-actions

github-actions Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Coverage: 91.58% (unit tier, Python 3.15, 5e27d3056, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19599 881 2618 867 92.11%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3029 23 1050 25 98.82%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17444 218 4588 185 98.15%
pcapkit/toolkit 914 36 334 10 95.51%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4432 1988 1018 167 51.87%

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

@JarryShaw
JarryShaw merged commit 1826870 into main Oct 27, 2026
61 checks passed
@JarryShaw
JarryShaw deleted the fix/2277-ipv4-ts-default branch October 27, 2026 04:25
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge at 5e27d3056. Closes #2277.

  • Review: NEEDS CHANGES → fixed (stale sample-generator comment) → self-certified, prose-only change.
  • CI: Required checks passed SUCCESS, 0 failures, KTLO clean.
  • Ordering leg: protocols gave 1963 tests with 0 failures, run with the PR merged into current main.
  • Merge state: mergeable.

@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 27, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 28, 2026
Merged
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(ipv4): TS option maker rejects its own default timestamp=None and prints it without !r

1 participant

@JarryShaw