Skip to content

fix(vendor): pass regex flags as flags=, not positional count (#796) - #813

Merged
JarryShaw merged 1 commit into
mainfrom
fix/796-re-sub-positional-flag
Sep 25, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/796-re-sub-positional-flag

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #796.

Thirteen re.sub sites under pcapkit/vendor/ passed re.MULTILINE (value
8) as the 4th positional argument, which is count, not flags. That
caps substitution at 8 matches and raises DeprecationWarning on Python
3.13+ (a future TypeError). Found by an AST sweep rather than grep, since
8 of the 13 put the flag on a continuation line.

Fixed by moving the flag to flags= at all 13 sites (default.py plus one
site each in hip/eddsa_curve.py, http/frame.py, http/method.py,
http/status_code.py, ipv4/router_alert.py, ipv6/option.py,
ipv6/router_alert.py, ipv6/tagger_id.py, reg/ethertype.py,
tcp/flags.py, tcp/mp_tcp_option.py, tcp/option.py). Behaviour
preserving: none of the 13 sites' shared pattern (r'\r*\n') carries a
MULTILINE-sensitive anchor, and every file under pcapkit/const/ still
parses cleanly, so no const/ regeneration is needed.

Adds tests/vendor/test_re_sub_positional_flag_unit.py: an AST sweep
(self-tested against known-positive/negative fixtures) plus runtime tests
that call each fixed method directly under DeprecationWarning-as-error.

…onal count (#796)

- Thirteen call sites under pcapkit/vendor/ passed re.MULTILINE (value 8)
  as re.sub's 4th positional argument, which is the count slot, not flags.
  This capped substitution at 8 occurrences and fires DeprecationWarning
  on Python 3.13+, becoming a TypeError in a later release.
- Found by an AST sweep of re.sub/re.split/re.subn calls rather than grep,
  since 8 of the 13 sites put the flag on a continuation line, invisible
  to a single-line pattern.
- Fixed by moving the flag to flags= at all 13 sites: default.py, and one
  site each in hip/eddsa_curve.py, http/frame.py, http/method.py,
  http/status_code.py, ipv4/router_alert.py, ipv6/option.py,
  ipv6/router_alert.py, ipv6/tagger_id.py, reg/ethertype.py, tcp/flags.py,
  tcp/mp_tcp_option.py, and tcp/option.py.
- Behaviour-preserving: none of the 13 sites' shared pattern (r'\r*\n')
  carries a MULTILINE-sensitive anchor, and every file under
  pcapkit/const/ still parses cleanly, so no const/ regeneration is
  needed.
- Adds tests/vendor/test_re_sub_positional_flag_unit.py: an AST-based
  regression sweep (self-tested against known-positive and
  known-negative fixtures) plus runtime DeprecationWarning pins.

Build: coverage run -m pytest tests/vendor tests/const passes, unit-tier.
@JarryShaw JarryShaw added bug fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict: GOOD TO GO (opus; author was sonnet). The 13 source changes are correct, minimal and complete. Every finding is about the argument and the test's scope, not the fix.

The reviewer wrote an independent sweep from the stdlib signatures — not from the PR's helper — and it agrees site-for-site: base 4530424df → 13 hits at exactly the author's line numbers, head → 0, over all 506 files in pcapkit/ rather than just vendor/'s 141. Self-tested against 6 known-positives (plain, continuation-line, re.split, re.subn with A|B, the short alias re.I, bare-import) and 9 known-negatives (including a genuine positional count, a genuine positional maxsplit, and a flag correctly placed in the 5th slot for both sub and split). Its sweep also catches two forms the PR's does not — the bare-import from re import sub shape, and restricting "flag" to the 18 real re.* flag names.

re.MULTILINE is a no-op for r'\r*\n' — proved exhaustively, not argued: 3,124 (string, count) pairs over ['a','\n','\r','\r\n','b'] up to length 4, 0 differences, with the anchored r'^\r*\n' differing on 288 of the same strings as a positive control.

Fail-before confirmed exactly and for the right reason: base gives FAILED (failures=1, errors=13) and all 13 errors are DeprecationWarning: 'count' is passed as positional argument — not an incidental AttributeError from the cls.__new__ construction — which is what proves each probe genuinely enters the code path. default.py's fix is inherited by 27 classes that define no process() of their own (AST-scanned; 95 override it).

The finding that matters, and it corrects my own issue text

#796's headline claim was factually wrong, and the PR's test for it cannot fail. I wrote in #796 that the generated const module "gets embedded newlines where it should have spaces" and that the damage is visible. It is not. All 13 sites feed Vendor.wrap_comment(), which is textwrap.wrap(text.strip(), 76) — and textwrap.wrap defaults to replace_whitespace=True, so any newline surviving the count=8 cap becomes a space before it reaches the generated file. Verified by me:

pre-fix re.sub output has newlines: True   |  post-fix: False
after wrap_comment, pre == post ?   True         <- the masking
SELF-TEST replace_whitespace=False would differ -> True

So the ast.parse-over-139-files proxy for "no regeneration needed" cannot fail on this defect and is not evidence. The conclusion survives by a stronger route — for \n-only breaks the output is provably identical at any count. The one real residual is \r\n past the 8th match, which leaves doubled spaces rather than newlines:

pre : 'x x x x x x x x x  x  x  x  TAIL'      post: 'x x x x x x x x x x x x TAIL'

The reviewer scanned all of const/ for that signature — 25 hits, every one traced to another cause: a [**]/[nn] footnote deleted by tcp/option.py's outer re.sub (3), a lone \r surviving name.replace('\n',' ') in reg/ethertype.py (1), and IANA's own intra-bracket whitespace in sctp/parameter.py and sctp/payload_protocol_identifier.py (21). So no evidence the cap ever bit — but not for the reason the issue gave. I am correcting #796 rather than leaving that in the record.

Three smaller findings, none blocking

  • The permanent sweep is scoped to pcapkit/vendor/ while the defect family is package-wide — fix(http): reach _guess_version's HTTP/2 arm, and resolve PayloadField names #789 fixed the identical shape in protocols/application/httpv1.py:310/:346. VENDOR_ROOT = ROOT / 'pcapkit' is a one-line widening and the test's own fixtures live under tests/, so it would still not self-trip.
  • A false precedent in the new docstring: it cites cls.__new__(cls) as used by both test_vendor_dest_path_unit.py and test_crawler_reachability_unit.py; the second contains zero occurrences. One precedent, not two.
  • test_pattern_has_no_multiline_anchor guards nothing — it asserts against the test file's own hardcoded PATTERN, never against the source's patterns, so a real call site gaining an anchor still passes.

Coverage reproduces in delta but not in absolute: 4241 stmts / 2588 → 2294 missed, 35% → 42%, against the author's 4198 / 2545 → 2251 and 35% → 43%. The delta is identical — 294 statements — and the totals differ by a uniform 43 statements uncovered in both, i.e. a scoping difference, not a discrepancy.

Proportionality: 386 lines for 13 one-line fixes is justified but padded — ~160 lines are prose, the durable ~60 are the sweep and its self-test, and the 13 runtime cases are near-duplicates a subTest table would have expressed in a third of the space. They break loudly rather than silently, and they buy the 294 statements. Merge as-is; fold the three findings into the body.

Unverifiable without network: whether regenerating const/ against live IANA today would change a byte. base/pcapkit/const and head/pcapkit/const are byte-identical, so the PR regenerates nothing.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw
JarryShaw merged commit 55e1b75 into main Sep 25, 2026
25 of 31 checks passed
@JarryShaw
JarryShaw deleted the fix/796-re-sub-positional-flag branch September 25, 2026 22:26
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Correcting my verdict here: I cleared this PR on incomplete CI, and it merged red on two Python legs.

main is now failing Python 3.11, Python 3.12, Engines Python 3.11 and Engines Python 3.12 — one defect, four legs. This PR's new tests/vendor/test_re_sub_positional_flag_unit.py:218 asserts a DeprecationWarning that CPython only emits from 3.13, with no version guard:

AssertionError: DeprecationWarning not triggered
measured: 3.14.7 warns | 3.11 and 3.12 do not
grep -nE "version_info|skipIf|skipUnless" over the file -> nothing

Not the author's fault. Its verification ran on 3.14, the documented local interpreter, and the file's own docstring at :25 states the boundary correctly — "a live DeprecationWarning on Python 3.13+". The matrix exists to catch precisely the gap between that sentence and the assertion, and I flipped this to good-to-go while six checks were still running. "No failures yet" is not "green".

Tracked as #819, with the fix shape: invert the assertion rather than skip it — assert the warning on 3.13+ and its absence below, so both branches stay pinned instead of two legs going dark. That mirrors what #788 did for typing.final's 3.11 boundary, which is the precedent to copy.

The rule I am applying from here: a review: good-to-go requires CI complete — inc=0, not merely fail=0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vendor: thirteen re.sub sites pass a regex flag as the positional count, capping substitutions at 8

1 participant