Skip to content

fix(vendor): make the LinkType legacy-sink rule value-aware, and correct its comment #852

Description

@JarryShaw

Description

Follow-ups from the cross-review of #848, which introduced the "sink rows whose notes mention legacy to
the end" rule in pcapkit/vendor/reg/linktype.py. All three were judged non-blocking for that PR and
are recorded here rather than lost. None is a live defect today — a live fetch of
http://www.tcpdump.org/linktypes.html (205 rows) matches the rule on exactly one row,
209 / LINKTYPE_IPMB_LINUX / "Legacy names (do not use) for Linux I2C below.".

1. The rule is not value-aware, so it can invert in the other direction

sink = legacy if 'legacy' in cmmt.lower() else enum tests the notes column for the word alone. It does
not check whether the value is already claimed by an earlier row.

If a future current row's notes say something like "supersedes the legacy DLT_FOO" while its legacy twin's
notes do not, the rule sinks the current row and re-creates #844 in mirror image — silently, since
nothing asserts which of a duplicated pair is canonical beyond value 209.

A value-aware predicate would be immune: sink only when the row's value is already present in enum.

2. The range branch inherits the same sink, so one row can move sixteen members

The ValueError branch that expands a range (USER0–USER15, values 147–162) uses the same sink
variable. A single range row whose notes mention "legacy" would therefore sink all sixteen members at
once, not one.

3. The generator's comment oversells what the code does

The comment describes the sunk rows as

a legacy alias sharing its value with a later, current row

but the implemented predicate never checks value-sharing — it only looks for the word. Either make the code
match the comment (which is item 1) or make the comment match the code.

4. Cosmetic: the generated file is no longer numerically ordered

IPMB_LINUX = 209 now sits after DECT_NR_TAP = 304 at the end of the class body, because sinking appends.
Nothing tests for numeric ordering and the canonical iteration order is positionally unchanged — verified,
one positional difference in 219 slots — but a reader scanning the table will trip over it. Worth a comment
at the emission site at least.

Acceptance

  • the predicate is value-aware, or the comment is corrected to describe what it actually tests
  • a test pins which member is canonical for a duplicated value in both directions — i.e. one that fails if
    a future regeneration sinks the current row instead of the legacy one
  • the range-branch interaction is either prevented or covered by a test

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    fixPull requests that fix a defect (fix: subject prefix)
    constRegenerated IANA or vendor constant tables; members keep their numeric values
    on Sep 27, 2026
  2. JarryShaw commented on Sep 27, 2026

    @JarryShaw
    OwnerAuthor

    Checkable blocker: #848 merged. Labelled blocked rather than dispatched, because the fix would
    collide with an open PR rather than merely follow it.

    #848 currently modifies both files this issue must change:

    gh pr view 848 -R JarryShaw/PyPCAPKit --json files -q '.files[].path'
      pcapkit/const/reg/linktype.py
      pcapkit/vendor/reg/linktype.py
      tests/const/test_const_linktype_209_unit.py
    

    The 'legacy' in cmmt.lower() predicate this issue makes value-aware is #848's diff — it is added by
    that PR, at pcapkit/vendor/reg/linktype.py. Starting here first would mean either rewriting code that is
    not on main yet, or a conflict when #848 lands. Verify the gate:

    gh pr view 848 -R JarryShaw/PyPCAPKit --json state -q .state    # MERGED == unblocked
    

    #848 is review: good-to-go on 88f52fd13 with 27 ✅ / 0 ❌ / 0 incomplete, so this should clear shortly.

    One thing to carry forward when it does: item 1 wants a test that fails if a future regeneration sinks the
    current row instead of the legacy one. That is a different assertion from the one #848 added, which pins
    only the present direction — so it is a genuine addition rather than a duplicate, and worth writing before
    touching the predicate.

  3. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 27, 2026
  4. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 27, 2026
  5. added this to the 1.5 milestone on Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)constRegenerated IANA or vendor constant tables; members keep their numeric valuesfixPull requests that fix a defect (fix: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions