Skip to content

fix(hip): resolve a str enum name in list-argument makers too - #2401

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2394-hip-list-argument-str-names
Oct 27, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/2394-hip-list-argument-str-names

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2394.

  • Problem: follow-up to fix: several makers reject a str enum name when their argument defaults to None #2356/fix(protocols): resolve a str enum name when its argument defaults to None (#2356) #2393, which fixed scalar _make_index arguments whose own default is None. A separate 12-site family in hip.py has the same symptom but a different shape — the str name is a loop variable over a list parameter (e.g. _make_param_dh_group_list(groups=['Group_384_bit_group'])) — so the member/int forms of the same call already worked while the str-name form raised ProtocolNotImplemented.
  • Fix: each of the 12 makers gets if X_namespace is None: X_namespace = Enum_Y hoisted above its build loop, next to the existing if X is None: X = []. Chosen over inlining the conditional at the _make_index call site because at that indentation it pushed 10 of 12 lines past 120 columns.
  • Two notes, not deviations: _make_param_transport_format_list's list holds parameter codes, so its namespace is Enum_Parameter — the same enum its own code argument carries. _make_param_reg_failed's scalar sibling failure_type already resolved by inference (its default is a real enum member); left alone.
  • Import promotion: 5 of the 9 needed enums (Enum_Suite, Enum_NATTraversal, Enum_Registration, Enum_ESPTransformSuite, Enum_Transport) were TYPE_CHECKING-only, next to Enum_RegistrationFailure from the same pcapkit.const.hip package already imported at runtime. Promoted all 5 — pcapkit/const/hip/*.py never imports from pcapkit/protocols, so this can't create a cycle; verified with a cold import of pcapkit.protocols.internet.hip as the very first import.
  • Test: tests/protocols/internet/test_hip_list_argument_str_names_2394_unit.py, 7 tests / 17 subtests — str/member/int parity for all 12 sites, a mixed list, the scalar sibling's inference, an explicit namespace override, an unknown-name raise, the 5 promoted enums, and an AST walk asserting every list-argument _make_index call in hip.py has a covering case.
  • Fail-without-fix: 19 failed / 5 passed against unfixed source (the 2 passing are structural/no-regression checks, correctly fix-insensitive); all pass with the fix.

Follow-up to #2356/#2393. That fix covered scalar _make_index arguments
whose own default is None, so the namespace couldn't be inferred. A
separate 12-site family in hip.py has the same symptom but a different
shape: the str name is a loop variable over a list parameter (e.g.
_make_param_dh_group_list(groups=['Group_384_bit_group'])), so the
member and int forms of the same call already worked while the str-name
form raised ProtocolNotImplemented.

Each of the 12 makers gets `if X_namespace is None: X_namespace = Enum_Y`
hoisted above its build loop, next to the existing `if X is None: X = []`
-- one decision per parameter rather than one per item, and every
original _make_index call line stays unchanged (an inline conditional at
that indentation pushed 10 of 12 lines past 120 columns).

_make_param_transport_format_list's list holds parameter codes, so its
namespace is Enum_Parameter -- the same enum its own code argument
carries, not a namespace of its own. _make_param_reg_failed's scalar
sibling failure_type already resolved by inference (its default is a
real enum member); left alone, since the two arguments there now resolve
against different enums.

5 of the 9 enums these makers need (Enum_Suite, Enum_NATTraversal,
Enum_Registration, Enum_ESPTransformSuite, Enum_Transport) were imported
only under TYPE_CHECKING, next to Enum_RegistrationFailure from the same
pcapkit.const.hip package already imported at runtime. Promoted all 5:
pcapkit/const/hip/*.py never imports from pcapkit/protocols, so this
cannot create a cycle -- verified with a cold import of
pcapkit.protocols.internet.hip as the very first import, not just
`import pcapkit`.

Tests: tests/protocols/internet/test_hip_list_argument_str_names_2394_unit.py,
7 tests / 17 subtests -- str/member/int parity for all 12 sites, a mixed
list, the scalar sibling's inference, an explicit namespace override, an
unknown-name raise, the 5 promoted enums, and an AST walk asserting every
list-argument _make_index call in hip.py has a covering case.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@JarryShaw
JarryShaw merged commit 027f7d5 into main Oct 27, 2026
64 checks passed
@JarryShaw
JarryShaw deleted the fix/2394-hip-list-argument-str-names branch October 27, 2026 22:33
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at 556a529ab (Sonnet) — arrived after merge, so this is now a post-merge follow-up rather than a revision request. Core fix fully verified: import-cycle claim holds (no const.hip → protocols imports, clean cold-import, no pylint R0401 cycle either direction), all 12 enum namespace choices cross-checked against the schema/reader side (136/136 str names round-trip correctly), the two flagged notes (transport_format_list's Enum_Parameter, reg_failed's unaffected scalar sibling) both confirmed correct, bug reproduces on base for all 12 sites and is fixed on head with byte-identical member/int output, 543 passed/11993 subtests, mypy/pylint unchanged, AST guard confirmed non-trivial (catches a planted 13th site).

One real issue: the new test file's prose and one assertion (assertEqual(len(found), 12)) violate the standing no-counted-numbers-in-docs ruling, which explicitly covers count pins and their tests. Filing a follow-up fix now.

Non-blocking finding from the review, worth knowing: with *_default set, origin/main silently encoded the default instead of the named value in most rows tested — not covered by this PR's scope, no test currently pins it either way.

JarryShaw added a commit that referenced this pull request Oct 27, 2026
test_hip_list_argument_str_names_2394_unit.py stated counts of the HIP
makers it covers ("twelve", "thirteenth", "five") in prose, and pinned
len(found) == 12 as an assertion -- both forbidden by the project's
state-no-count convention (docs/source/contributing/conventions/
documentation.rst), which extends to count pins and the tests that check
them, not just narrative docs. Found by #2401's own cross-review, which
landed after that PR had already merged.

Reworded the 8 affected lines to describe shape rather than count (e.g.
"every one of them" rather than "all twelve"). Dropped the len(found)
count assertion: it was redundant with the adjacent
assertEqual(found, set(CASES)), whose set equality already forces the
same length check, with a named maker in the failure message, and
without a number that goes stale the moment a 13th site is added.

No behavior change; 7 passed / 17 subtests, same as before.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
test_hip_list_argument_str_names_2394_unit.py stated counts of the HIP
makers it covers ("twelve", "thirteenth", "five") in prose, and pinned
len(found) == 12 as an assertion -- both forbidden by the project's
state-no-count convention (docs/source/contributing/conventions/
documentation.rst), which extends to count pins and the tests that check
them, not just narrative docs. Found by #2401's own cross-review, which
landed after that PR had already merged.

Reworded the 8 affected lines to describe shape rather than count (e.g.
"every one of them" rather than "all twelve"). Dropped the len(found)
count assertion: it was redundant with the adjacent
assertEqual(found, set(CASES)), whose set equality already forces the
same length check, with a named maker in the failure message, and
without a number that goes stale the moment a 13th site is added.

No behavior change; 7 passed / 17 subtests, same as before.
@JarryShaw JarryShaw removed the review: running A cross-review is in flight against the current head - no verdict yet label Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(hip): 12 list-argument makers can't resolve a str enum name either

1 participant