Skip to content

vendor: three bespoke templates and AppType's dunders still use %-formatting, so the f-string disable cannot drop #798

Description

@JarryShaw

Describe the bug

#792 converted the six %-style raises in pcapkit/const/reg/apptype/apptype.py to f-strings, under the maintainer's convention from #783: "id like to keep f-string convention across the library. only use % substitution when inevitable." Three parts of that convention are still unmet, and nothing was tracking them until now.

1. Three other bespoke templates still raise with %. tests/const/test_const_enum_builtin_parity.py's BESPOKE_TEMPLATES names four modules that each carry their own copy of #647's _missing_ guard. #792 converted one; these three still hold the % form:

pcapkit.vendor.tcp.flags
pcapkit.vendor.ftp.command
pcapkit.vendor.http.method

Each is raise ValueError('%r is not a valid %s' % (value, cls.__name__)).

2. Three %-formatted dunders in the AppType template. __new__, __repr__, __str__. #792 left these deliberately and the reasoning is sound — __new__'s format sets the underlying StrEnum value for every one of ~12,391 real members, a far larger blast radius than an error path — so this wants care, not a sweep. Measure the emitted values before and after and prove them byte-identical, member by member.

3. The # pylint: disable=consider-using-f-string cannot drop until 1 and 2 are done, and also needs the extend_enum(..., '%d' % value, ...) calls in the span-handling tail converted. That disable being earned is the cheapest signal the work is unfinished.

Expected behavior

f-strings across all four bespoke templates and the AppType dunders, with the module-level disable dropped once nothing needs it.

Additional context

This issue exists because I cited the wrong one. #792's PR carries a comment reading "#796 tracks sweeping them" — but #796 is vendor: thirteen re.sub sites pass a regex flag as the positional count, an unrelated defect. I wrote that mis-citation into the worker's brief. The comment is being corrected to point here.

Also correcting #792 itself. Its "Expected behavior" said "f-strings throughout, and the disable dropped", which over-reached past its own title — that title scopes to "six pre-existing %-style raises". #792 is being narrowed to match, so it can close on the PR that does the six without its stated criteria going unmet. The remainder is this issue.

Two traps, both measured on #792's work:

  • The templates are generated, so edits go in pcapkit/vendor/..., where BASE is a lambda returning an f-string — braces in the emitted code must be doubled. :243's = {{}} is the precedent.
  • Do not regenerate. AppType.LINK fetches IANA's live CSV. Hand-edit and prove equivalence by rendering BASE(...) and diffing the changed region, expecting 0 diff lines.

And the lesson from #792's own 7-leg CI failure: grep tests/ for the literal text of every line touched, not only the constructs judged risky. A test pinned the guard's % text and nothing flagged it until CI went red — and note both tests in that class are skipUnless(find_spec('requests')), so they skip silently without the vendor extra.

Related: #792, #783, #647, #796.

Activity

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

    constRegenerated IANA or vendor constant tables; members keep their numeric valuesrefactorRestructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions