refactor(const): f-string the last three %-formatted __repr__ methods (#804)

[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 — tests cover it, but make test was not run in full; tests/const and tests/vendor only

• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — changelog 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
• chore — anything else

---

Description of your pull request and other information

Closes #804. The three __repr__ methods #798 left on % — pcapkit/const/ftp/command.py:40,119 and pcapkit/const/http/method.py:42, re-derived at 55e1b756e — are f-strings now, so consider-using-f-string drops from both files and from both pcapkit/vendor/ templates. Each generator's own %-formatted wrap_comment argument went with them, dropping their two inline disables: nothing in this pair needs it any more. The other bespoke templates in these directories ({const,vendor}/ftp/return_code.py, {const,vendor}/http/status_code.py, vendor/http/frame.py) and the shared vendor/default.py one still carry it, so the issue's "nothing in pcapkit/{const,vendor}/{ftp,http}/ needs it" is met for this pair only, not for those directories.

Generated, so both sides changed. Rendering each LINE template with the real arguments reproduces its committed const module byte-for-byte; diffing the render against 55e1b756e gives 6 changed lines for ftp/command and 4 for http/method — the intended pairs, nothing else. No crawl: the enumeration block is read back out of the committed module.

The conversions are inert, derived rather than assumed — #796's text claimed an equivalence in generated output that wrap_comment's textwrap.wrap had masked. repr() is asserted member by member against the old % expression's own output for all of Command, FEATCode and Method, including a desc of None; both generators' process() output is asserted the same way, on synthetic rows, over both the truthy and falsy rfcs branch. pylint (Makefile flags plus useless-suppression), mypy and isort report exactly the same findings as 55e1b756e, and no useless-suppression for the dropped disable either side.

Tests. tests/const/test_const_enum_builtin_parity.py 22 → 27 tests. test_the_disable_drops_only_where_nothing_else_needs_percent_formatting asserted the opposite for this pair — that the disable was retained, and why — so it is flipped rather than added to. Against stock 55e1b756e 8 subtests across 3 methods fail; after, 27 tests / 701 subtests pass, agreeing under python -m unittest (27 OK). The %-format detector is an ast walk over BinOp(Constant(str) % x), self-tested on 3 known-positives and 4 known-negatives first: the issue's own grep '[^']*%[sdr] reads 0 on these files because every surviving line was double-quoted. Note that walk reports 1 % site per vendor module where the issue's table said 3 and 2 — inside a template the __repr__ lines are string content, not expressions.

Coverage over tests/const + tests/vendor (146 → 151 tests): const/ftp/command.py 96.800% → 100.000%, const/http/method.py 95.833% → 97.222%, vendor/ftp/command.py 26.316% → 78.947%, vendor/http/method.py 83.019% → 86.792%. All three converted __repr__ lines were previously uncovered.

[JarryShaw]

Both of the author's corrections to #804's text are right, and both were my errors. Verified:

1. My "nothing in pcapkit/{const,vendor}/{ftp,http}/ needs it" over-claimed. Twelve files there carry the disable on 55e1b756e, not the four in scope:

const/ftp/command.py  const/ftp/return_code.py   const/http/error_code.py  const/http/frame.py
const/http/method.py  const/http/setting.py      const/http/status_code.py
vendor/ftp/command.py (x2)  vendor/ftp/return_code.py  vendor/http/frame.py
vendor/http/method.py (x2)  vendor/http/status_code.py (x2)

So this PR earns the disable for its pair, which is the honest claim, and eight other files still carry it. Filing that remainder separately rather than widening this PR.

2. My table's %-format counts were textual, not structural. I wrote 3 for vendor/ftp/command.py and 2 for vendor/http/method.py. An AST walk over BinOp(Constant(str) % x) gives 1 each:

vendor/ftp/command.py   AST %-BinOp=1   textual lines with %s/%d/%r=3
vendor/http/method.py   AST %-BinOp=1   textual lines with %s/%d/%r=2
SELF-TEST: AST finds a real one -> 1

Inside a LINE = lambda …: f''' template the __repr__ lines are string content, not expressions. That is the same error as the single-quote grep that reported 0 on these files — counting text where structure was the question. Third instance today of a textual count standing in for a structural one.

The load-bearing test find, and it was handled the right way. tests/const/test_const_enum_builtin_parity.py:658 asserted the disable was RETAINED for this exact pair — assertIn('consider-using-f-string', source) together with assertIn('%', source). That is a test which must break, and it was flipped rather than deleted, which is the distinction I brief for every time.

Verification I accept as done: template render-and-diff byte-identical with diffs of exactly 6 and 4 lines (the two __repr__ pairs plus the disable pairs), useless-suppression under the real Makefile flags showing zero C0209 after and no spurious flag before, findings identical before/after at 16 with only two E1101 column offsets moving, and 8 subtests across 3 methods failing on stock. Coverage: const/ftp/command.py 96.8% → 100%, vendor/ftp/command.py 26.3% → 78.9% — all three converted __repr__ lines were previously uncovered.

Its %-detector was self-tested against 3 known-positives and 4 known-negatives, and it pins why my grep read zero: re.search(r"'[^']*%[sdr]", 'x = "<%s [%s]>" % (a, b)') is None. Good — that turns my mistake into a regression guard.

Also correct and worth crediting: the make test checklist box is left unticked with the reason inline, rather than ticked on assumption. That is the behaviour I asked for after a ticked-but-unrun box on #811.

Cross-review dispatching now, on a model other than the one that authored this.

[JarryShaw]

Cross-review verdict: GOOD TO GO (sonnet; author was haiku). Every item confirmed, several exactly. No unprompted findings above informational.

The test flip is strictly stricter, which was the thing to check — verified by me:

stock: dropped = (2 modules, assertNotIn)  +  retained = (2 modules, assertIn + assertIn('%'))
head:  dropped = (4 modules, assertNotIn)  —  the retained block is gone
assertion counts: stock 1 assertIn + 1 assertNotIn  ->  head 0 assertIn + 2 assertNotIn

The two modules moved from "must keep the disable" to "must not have it", joining the other two — so the guard now covers 4 modules under the stronger assertion instead of 2 under each. Nothing in the +269/−19 touches any other existing assertion: the diff is exactly 3 hunks, two of them pure additions.

The tests genuinely ran, and the reviewer proved the gate distinguishes ran from skipped — the check I care most about here, since both tests are skipUnless(find_spec('requests')) and a silent skip is how the earlier 7-leg breakage reached CI. Ran 27 tests … OK with both render tests showing ... ok; then monkeypatching find_spec('requests') → None flipped both to ... skipped 'pcapkit.vendor needs requests'.

Pylint confirmed character for character: 16 findings on both sides, zero C0209, and the only difference is two column offsets — const/ftp/command.py:40 55→46 and :119 57→45 — matching the body exactly. Run with the literal PYLINT_FLAGS plus --enable=useless-suppression, not the --disable=all form that manufactures hits.

Render-and-diff re-derived independently (its own script, not the test's assertions): 6 changed lines for const/ftp/command.py, 4 for const/http/method.py — the disable pair plus the repr pairs, nothing else — with a known-negative that mutated one __repr__ in the rendered output and was correctly detected.

Fail-before exactly: Ran 10 tests … FAILED (failures=8), the 8 subTests spanning exactly the 3 named methods. And it confirmed the 3 inertness proofs pass on stock too and are honestly excluded from the 8 rather than counted as fail-before.

Proportionality judged, and I agree with the reasoning: +269 test lines for a 14-line source change is proportionate because each new test closes a failure mode this file family has actually suffered — a quote-blind grep (now a self-tested detector), a half-applied template/const edit (render-and-diff), an assumed-but-unverified output equivalence (#796's own mistake, now proven member by member), and a silently-skipped render test.

One informational, pre-existing on both sides: const/http/method.py:2 carries # pylint: disable=line-too-long which useless-suppression flags as I0021 — a stale disable of a different rule, unrelated to this PR and not a regression. Folding it into #818 rather than widening this PR.

UNVERIFIED: the mypy/isort identical-before-after claim, and the author's statement-only coverage decimals (the reviewer measured branch-inclusive instead, matching in direction and magnitude).

Flipping to review: good-to-go.

[JarryShaw]

The four red legs are NOT this PR's fault — they are inherited from main, and I have filed #819 for them.

PR 817 FAILED: Python 3.11 / Python 3.12 / Engines Python 3.11 / Engines Python 3.12

One defect, four legs. tests/vendor/test_re_sub_positional_flag_unit.py:218 — a file this PR does not touch, added by #813 — asserts a DeprecationWarning that CPython only emits from 3.13, ungated:

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

This PR is based on 55e1b756e, which contains #813, so it inherits the breakage. #814 and #815 are based on 4530424df and therefore do not carry it — which is why they are still 27✅.

The verdict on this change stands at review: good-to-go — its own diff is sound and independently verified. But it cannot be merged green until #819 lands, so treat #819 as its practical blocker.

This is my error, and worth stating plainly: I cleared #813 as good-to-go on incomplete CI. It read 27✅ 0❌ when I looked, but the 3.11/3.12 legs had not finished. "No failures yet" is not "green", and I have been drawing that distinction for findings all session while failing to draw it for checks. The rule I am applying from here: a review: good-to-go requires CI complete, not merely non-failing — inc=0, not just fail=0.

[JarryShaw]

CI tests are failing (seems like failing on main branch as well, can be fixed with a direct push there).

[JarryShaw]

Ruling recorded, in your words:

CI tests are failing (seems like failing on main branch as well, can be fixed with a direct push there).

Confirmed, and it is now half done by your own merge: #820 landed as 319da66bb at 01:25:26Z, which removes the #819 defect — the re.sub positional-flag test — from main. That is exactly the defect these six legs are red on, so this PR goes green on a re-run or rebase onto 319da66bb.

I did not take the direct push, deliberately. The one remaining red on main is #822 (10 legs, all five Pythons, test_guess_version_reports_a_preface_with_no_frame_as_such), and its fix is open at #823 — test-only, one file, cross-reviewed GOOD TO GO, CI complete. I had the cherry-pick staged in a scratch worktree and dropped it: you are merging PRs yourself right now, and pushing the same diff directly would leave your merge of #823 either empty or conflicting. Merging #823 has the identical effect on main and keeps the review trail.

If you would rather I push it straight to main instead of you merging #823, say so and I will — it is test-only, so it is inside the standing authorisation either way.

[JarryShaw]

Rebased onto main (21e9588af) and force-pushed: ce992bcca → ed1240dcf.

This is the direct answer to your note here: the six red legs were #819, and your merge of #820 took that defect off main as 319da66bb. #822's defect went too, as 21e9588af. So this PR's CI should now be clean rather than inheriting anything.

review: good-to-go carries over because the rebase is content-identical — diff of the pre- and post-rebase diffs is empty. One commit, same five files.

[JarryShaw]

CI is complete and clean — ok=27 fail=0 inc=0 on ed1240dcf — so this is ready to merge.

The six legs that were red here are gone: they were #819, and your merge of #820 took that defect off main as 319da66bb. I rebased this branch onto 21e9588af afterwards, and the rebase was content-identical (diff of the pre- and post-rebase diffs is empty), which is why the GOOD TO GO verdict carries rather than needing a fresh review.

main itself is now green too — run 36208905786 on 21e9588af finished 18 jobs success, 0 failed, 1 skipped. That is the first clean main since 23:38Z, and it confirms both #819 and #822 are cleared.

One thing to know before merging, since it decides the order: #821 and #826 both touch pcapkit/protocols/application/http.py, so whichever of those two goes second will need a rebase. This PR touches neither, so it is independent of that ordering.