fix(vendor): pass regex flags as flags=, not positional count (#796) - #813
Conversation
…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.
|
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
Fail-before confirmed exactly and for the right reason: base gives 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 So the The reviewer scanned all of Three smaller findings, none blocking
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 Unverifiable without network: whether regenerating |
|
Correcting my verdict here: I cleared this PR on incomplete CI, and it merged red on two Python legs.
Not the author's fault. Its verification ran on 3.14, the documented local interpreter, and the file's own docstring at 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 The rule I am applying from here: a |
make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Closes #796.
Thirteen
re.subsites underpcapkit/vendor/passedre.MULTILINE(value8) as the 4th positional argument, which iscount, notflags. Thatcaps substitution at 8 matches and raises
DeprecationWarningon Python3.13+ (a future
TypeError). Found by an AST sweep rather than grep, since8 of the 13 put the flag on a continuation line.
Fixed by moving the flag to
flags=at all 13 sites (default.pyplus onesite 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). Behaviourpreserving: none of the 13 sites' shared pattern (
r'\r*\n') carries aMULTILINE-sensitive anchor, and every file under
pcapkit/const/stillparses 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.