Skip to content

fix(protocols)!: make NTPTimestamp and PMIPv6Timestamp Info classes (#1886) - #1887

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1886-mh-timestamps-info
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1886-mh-timestamps-info

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

As ruled on #1869, NTPTimestamp and PMIPv6Timestamp become Info classes. Breaking: NTPTimestamp shipped as a tuple in every 1.4.x release.

  • New home. Both move to pcapkit/protocols/data/internet/mh.py as Data subclasses, modelled on hip.Lifetime. That is where every nested Info held by a data model lives. It is also the only place Info.from_dict can rebuild them: from mh.py, the import cycle makes from_dict(to_dict()) return an OrderedMultiInfo.
  • No alias at the old path. NTPTimestamp gets versionremoved there. PMIPv6Timestamp was beta-only, so it gets none.
  • What changes for users. The timestamps are no longer tuples: indexing, unpacking, tuple equality and hashing are gone, and .seconds/.fraction still work. The dumpers now write {"seconds": …, "fraction": …}. versionchanged:: 1.5.0 notes this on both classes and on mh.rst.
  • Tuple uses fixed. mh.py:8985 unpacked pmip_timestamp; it now reads attributes. The tests that used tuple(...) do the same.
  • Changelog. The 1.5.0 changelog's claim that no NamedTuple remains is corrected, since TCPSegment stays one, and CHANGELOG.md is regenerated to match.

tests/protocols/internet/test_mh_timestamp_info_1886_unit.py covers Info behaviour, byte-exact parse→rebuild, field-keyed JSON and plist output, and pickling at every protocol. It fails 18 subtests on main. Full tests/project: 1005 passed.

Closes #1886

…1886)

- Define both as Data models in protocols.data.internet.mh, beside the options
  that hold them, so from_dict resolves their annotations and rebuilds them;
  export them as MH_NTPTimestamp and MH_PMIPv6Timestamp
- Read the Timestamp option's pair by attribute when rebuilding it, since an
  Info unpacks to its keys
- Add versionchanged on both classes, the module and its docs page, and
  versionremoved for NTPTimestamp's 1.4.x path
- Correct the 1.5.0 changelog's claim that no NamedTuple remains, since
  TCPSegment is one, and regenerate CHANGELOG.md from it
- Point the tests at the new home and register both names as moved

BREAKING CHANGE: NTPTimestamp is no longer a tuple, and protocols.internet.mh
no longer binds it; the dumpers write both timestamps keyed by field.

Closes #1886
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at ab26e6f81. A Sonnet cross-review checked the change against your ruling on #1869, and I checked the parts it didn't reach.

  • The move is needed. Keeping the classes in internet/mh.py makes from_dict(to_dict()) return OrderedMultiInfo for both fields, which the reviewer measured. In the data module the classes are rebuilt, as hip.Lifetime already is.
  • No re-export. pcapkit.protocols.internet.mh has no NTPTimestamp/PMIPv6Timestamp. Its private Data_* import follows the module's existing pattern. The moved-name registry is updated, with only NTPTimestamp marked as shipped.
  • Round trips are byte-exact. BRR messages carrying each option were built with make() from positional and keyword timestamps across four value sets, and rebuild byte for byte from info and from info.to_dict(). Out-of-range values are still rejected.
  • The docs are accurate. PMIPv6Timestamp first appears in v1.5.0b1 and never reached a final release. NTPTimestamp is in every 1.4.x tag.
  • Nothing is left relying on tuples. I grepped pcapkit/, tests/, docs/, examples/ and util/.
  • The changelog sentence is true. TCPSegment is the only typing.NamedTuple left, and VersionInfo/ModuleDescriptor are collections.namedtuple.
  • The tests are real. The new test fails 18 subtests on main, and putting the old unpacking back at mh.py:8985 fails 5 tests across three files.

Not blocking: the byte-exact round-trip test also passes on main, so only the from_dict type check shows why the move was needed.

@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 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 90.05% (unit tier, Python 3.15, ab26e6f81, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19595 881 2618 867 92.11%
pcapkit/corekit 2462 69 780 35 96.55%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2914 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17411 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 512 2 158 4 99.10%
pcapkit/vendor 4407 2342 1010 157 43.27%

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

@JarryShaw
JarryShaw merged commit fa2a9ce into main Oct 17, 2026
64 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 17, 2026
@JarryShaw
JarryShaw deleted the fix/1886-mh-timestamps-info branch October 17, 2026 13:27
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

protocols(mh): make NTPTimestamp and PMIPv6Timestamp Info classes

1 participant