[JarryShaw]

Please follow the guide below

• You will be asked some questions, please read them carefully and answer honestly

• Put an x into all the boxes [ ] relevant to your pull request (like that [x])

• Use Preview tab to see how your pull request will actually look like

• Searched for similar pull requests

• Followed the coding style (make pylint, make mypy, make isort)

• make test passes, and a test case covers the change. Narrow runs only, listed below.

• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible: N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

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

---

• Name the generated namedtuple bases of VersionInfo, ModuleDescriptor,
  NTPTimestamp and PMIPv6Timestamp with a leading underscore, so a page's
  Bases: line no longer links the class to itself. repr, str, copies,
  _make and the pickle bytes at every protocol are unchanged.
• Add a regression test that each direct namedtuple base is named apart
  from its class, and a guard that repr, copies and pickles still name the
  subclass.

Closes #1843

[JarryShaw]

Verdict: GOOD TO GO at 87610cb2a. A Sonnet cross-review (author: Opus) checked the following:

• Pickles: it pickled all four classes at protocols 0–5 on main and on this branch and loaded each on the other. Every load came back as the subclass, equal to the original, with identical bytes.
• Unchanged behaviour: repr, str, _fields, _asdict, __match_args__, _make, _replace, copy, deepcopy and __slots__ are all unchanged. Only __mro__, __bases__ and the base's own docstring differ.
• Name dependence: nothing in pcapkit, tests, docs, examples or util relies on the base's name.
• The test: its base-name cases fail on main and pass here.
• Coverage: grep namedtuple( finds exactly these call sites.

The bases now render as unlinked _VersionInfo and the like. The docs build is not nitpicky, so this adds no warning.

[JarryShaw]

For those named tuples, should they better be an Info class? VersionInfo stays named tuple as it’s matching the std-lib's, and ModuleDescriptor makes sense as well, but what about the others? Also, I don't think they'd need their specific name for the base classes? It's fine to be linked back to themselves as the base classes are actually just a placeholder.

[JarryShaw]

Thanks. My reading of your points:

• VersionInfo and ModuleDescriptor stay named tuples.
• You're asking whether NTPTimestamp and PMIPv6Timestamp would be better as Info classes.
• A base class linking back to its own class is fine, since the base is only a placeholder, so the underscore renaming this PR does isn't needed.

Before recommending, I'm checking what converting the two timestamps would break: tuple indexing and unpacking, comparison, hashing, pickling, and where they are documented and constructed. I'll reply here with the measured answer. I'm also checking which other named tuples in the package the same question applies to. Until then this PR is on hold, labelled review: needs-changes.

[JarryShaw]

Measured on main (0de96b457):

• NTPTimestamp has shipped as a tuple since 1.0.1.post2, through every 1.4.x release, so converting it is breaking. PMIPv6Timestamp arrived with protocols: complete the Mobility Header registry -- all 24 message types, 70 of 71 options, all 4 CGA extensions #437 and is beta-only.
• Neither has any methods. Every in-tree read is by attribute, except mh.py:8985, which unpacks seconds, fraction = option.pmip_timestamp. As an Info that unpacking yields the key names and raises TypeError, so that line and two tests need rewriting.
• What an Info loses: indexing, unpacking, == (s, f), hashing, ordering and _replace/_asdict. Nothing in-tree relies on any of them.
• What it gains: the dumpers write field names, {"seconds": …, "fraction": …}, where today they write {"type": "NTPTimestamp", "value": [1, 2]}. That is a change in output too.
• TCPSegment (toolkit/pcap.py) is also a named tuple. I'd keep it: it is built once per TCP frame and only read by attribute, and constructing an Info measured about 10x slower. It does make the 1.5.0 changelog's claim that no NamedTuple remains in the package false.

Recommendation:

1. Convert both timestamps to Info in one new PR, labelled breaking, and fix that changelog line.
2. Since you're fine with the self-link, close this PR unmerged and close fix(corekit): VersionInfo, ModuleDescriptor and the MH timestamp tuples list themselves as their own base #1843 as not planned. Its guard test passes on main without a fix, and it would pin the tuple behaviour the conversion removes.

Shall I go ahead with 1 and 2?

[JarryShaw]

take your reommendations

[JarryShaw]

Recorded: you accepted both recommendations.

• Closing this PR unmerged, and closing fix(corekit): VersionInfo, ModuleDescriptor and the MH timestamp tuples list themselves as their own base #1843 as not planned. A base class that links back to its own class is fine.
• The conversion of NTPTimestamp and PMIPv6Timestamp to Info is tracked in a new issue. It is a breaking change, and that issue also fixes the 1.5.0 changelog's claim that no NamedTuple remains in the package.