From ae6d22f2afedd5e48c550979cf137d1264efcbbe Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 25 Sep 2026 12:42:36 -0400 Subject: [PATCH] refactor(const): convert AppType's six %-style raises to f-strings (#792) The generated `pcapkit.const.reg.apptype.apptype` module had 6 `raise ValueError(...)` calls still using `%` formatting, left that way by #783 so its own diff over a 12,391-member file stayed reviewable. Per the maintainer's f-string convention (#783), they now match the other 605 f-string raises tree-wide: - `TransportProtocol._missing_`: `'%r is not a valid %s' % (...)` -> `f'{value!r} is not a valid {cls.__name__}'` - `AppType.__new__`: the "holds no members" guard - `AppType._dispatch`: the non-port `key` guard and the trailing "names no transport protocol registry" guard - `AppType._missing_`: both range/registry guards (same message as the first bullet) Each message was verified byte-identical old vs new (same repr()/str() semantics for %r/!r and %s/{}), and exercised through its real call path (`AppType.get('http', proto=tcp)`, `AppType.get(80, proto=undefined)`, `extend_enum(AppType, ...)`, out-of-range `AppType(...)`) with matching live exception text before and after. The edit is in the `BASE` template of `pcapkit/vendor/reg/apptype/apptype.py` (braces doubled, as an f-string template emitting f-strings) and applied by hand to the const file rather than by a crawl, to avoid rewriting the IANA-derived member rows. Expanding `BASE` with dummy TABLE/MISS and diffing against the const file shows exactly 2 non-equal regions -- the docstring's list-table and the range-row tail of `_missing_`, both TABLE/MISS-dependent -- and 0 elsewhere. Left unconverted, and said why in the PR: the three %-formatted dunders (`__new__`, `__repr__`, `__str__`) the issue also mentions -- out of the issue's stated raise-only scope, and `__new__`'s format string sets every member's underlying `StrEnum` value across all ~12,391 real members, a much larger blast radius than an error path. The module-level `consider-using-f-string` pylint disable stays: those dunders and the `extend_enum(..., '%d' % value, ...)` calls still use `%`. `tests/const/test_const_enum_builtin_parity.py::test_every_bespoke_template_ carries_the_guard` (GitHub issue #647) pinned all four bespoke vendor templates' guards against one shared `%`-style literal, so converting `reg.apptype.apptype`'s copy broke it in CI (7 legs). `BESPOKE_TEMPLATES` is now a dict keyed to each template's own guard text -- the other three still raise with `%` (#798 tracks sweeping them, the `%`-formatted dunders, and dropping the disable) -- rather than an `or` of both forms, which would let a genuinely deleted guard pass. Proved by deleting the guard from `pcapkit.vendor.tcp.flags` locally: the test failed with the expected subTest and message, then the file was restored and the tree verified clean. Build/test: `tests/const/test_const_apptype_split_unit.py` (19), `tests/vendor/test_vendor_reg_apptype_generator_unit.py` (6), and `tests/const/test_const_enum_builtin_parity.py` (20, `requests` importable so neither guard test skips) all pass under both pytest and plain unittest. mypy clean before and after. pylint: 0 new findings in either touched apptype file; R0801 unchanged at 575 tree-wide. Closes #792 --- pcapkit/const/reg/apptype/apptype.py | 16 ++++----- pcapkit/vendor/reg/apptype/apptype.py | 16 ++++----- tests/const/test_const_enum_builtin_parity.py | 35 +++++++++++++------ 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index 76cd73e5e..e2c16d2ef 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -90,7 +90,7 @@ def _missing_(cls, value: 'int') -> 'TransportProtocol': """ if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{value!r} is not a valid {cls.__name__}') return super()._missing_(value) @@ -2306,8 +2306,8 @@ def __new__(cls, value: 'int', name: 'str' = '', # two services on one port stay two canonical members instead of one # member and an alias -- an alias would answer to the other's name. if cls.__registry__ is None: - raise ValueError('%s holds no members; they belong to its per-transport ' - 'subclasses' % cls.__name__) + raise ValueError(f'{cls.__name__} holds no members; they belong to its per-transport ' + 'subclasses') cls.__registry__.add(value, obj) return obj @@ -2372,7 +2372,7 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[AppTyp # non-port outright is the honest answer, and it has to happen before the # miss path, which formats ``key`` with ``%d``. if not isinstance(key, int): - raise ValueError('%r is not a valid port number for %s' % (key, cls.__name__)) + raise ValueError(f'{key!r} is not a valid port number for {cls.__name__}') if cls.__registry__ is not None: return cls @@ -2417,8 +2417,8 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[AppTyp subclass = cls.__registries__.get(TransportProtocol(namespaces[0])) if subclass is not None: return subclass - raise ValueError('%r names no transport protocol registry of %s' - % (proto, cls.__name__)) + raise ValueError(f'{proto!r} names no transport protocol registry of ' + f'{cls.__name__}') @classmethod def get(cls, key: 'int', *, @@ -2525,13 +2525,13 @@ def _missing_(cls, value: 'int') -> 'Optional[AppType]': """ if not (isinstance(value, int) and 0 <= value <= 65535): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{value!r} is not a valid {cls.__name__}') # NOTE: extending this class would give it a member, and aenum then # refuses to subclass it -- permanently, for every registry not yet # imported. The spans below belong to whichever registry was asked, never # to this one. if cls.__registry__ is None: - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{value!r} is not a valid {cls.__name__}') # NOTE: most spans are IANA's unassigned and reserved markers, which name # no transport protocol and so answer every registry. A span that does name # one tests ``cls.__transport__`` and answers that registry alone -- diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index 2b8ca27ed..b6b52cf14 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -183,7 +183,7 @@ def _missing_(cls, value: 'int') -> 'TransportProtocol': """ if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{{value!r}} is not a valid {{cls.__name__}}') return super()._missing_(value) @@ -261,8 +261,8 @@ def __new__(cls, value: 'int', name: 'str' = '', # two services on one port stay two canonical members instead of one # member and an alias -- an alias would answer to the other's name. if cls.__registry__ is None: - raise ValueError('%s holds no members; they belong to its per-transport ' - 'subclasses' % cls.__name__) + raise ValueError(f'{{cls.__name__}} holds no members; they belong to its per-transport ' + 'subclasses') cls.__registry__.add(value, obj) return obj @@ -327,7 +327,7 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[{NAME} # non-port outright is the honest answer, and it has to happen before the # miss path, which formats ``key`` with ``%d``. if not isinstance(key, int): - raise ValueError('%r is not a valid port number for %s' % (key, cls.__name__)) + raise ValueError(f'{{key!r}} is not a valid port number for {{cls.__name__}}') if cls.__registry__ is not None: return cls @@ -372,8 +372,8 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[{NAME} subclass = cls.__registries__.get(TransportProtocol(namespaces[0])) if subclass is not None: return subclass - raise ValueError('%r names no transport protocol registry of %s' - % (proto, cls.__name__)) + raise ValueError(f'{{proto!r}} names no transport protocol registry of ' + f'{{cls.__name__}}') @classmethod def get(cls, key: 'int', *, @@ -480,13 +480,13 @@ def _missing_(cls, value: 'int') -> 'Optional[{NAME}]': """ if not ({FLAG}): - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{{value!r}} is not a valid {{cls.__name__}}') # NOTE: extending this class would give it a member, and aenum then # refuses to subclass it -- permanently, for every registry not yet # imported. The spans below belong to whichever registry was asked, never # to this one. if cls.__registry__ is None: - raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + raise ValueError(f'{{value!r}} is not a valid {{cls.__name__}}') # NOTE: most spans are IANA's unassigned and reserved markers, which name # no transport protocol and so answer every registry. A span that does name # one tests ``cls.__transport__`` and answers that registry alone -- diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index 83e855bee..805fa91e7 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -98,12 +98,24 @@ #: The bespoke templates under :mod:`pcapkit.vendor` that carry their own copy of #: the guard, rather than inheriting the one in :mod:`pcapkit.vendor.default`. #: Each was rendering a registry that GitHub issue #647 found unguarded. -BESPOKE_TEMPLATES = ( - 'pcapkit.vendor.tcp.flags', - 'pcapkit.vendor.ftp.command', - 'pcapkit.vendor.http.method', - 'pcapkit.vendor.reg.apptype.apptype', -) +#: +#: Keyed to each template's own guard text rather than one literal shared by +#: all four: GitHub issue #792 moved ``pcapkit.vendor.reg.apptype.apptype``'s +#: copy to an f-string, following the library-wide convention GitHub issue +#: #783 settled, while the other three still raise with ``%`` -- #792 +#: deliberately left them alone so the ``const`` diff stayed reviewable, and +#: #798 tracks sweeping them, along with the ``%``-formatted dunders and +#: dropping the f-string disable. One shared literal can no longer pin all four; +#: what #647 actually needs pinned is that each template still carries *a* +#: guard rejecting an invalid value, in whatever form that template's own +#: raise takes, not that the four agree on a formatting style the library is +#: moving away from. +BESPOKE_TEMPLATES = { + 'pcapkit.vendor.tcp.flags': "raise ValueError('%r is not a valid %s' % (value, cls.__name__))", + 'pcapkit.vendor.ftp.command': "raise ValueError('%r is not a valid %s' % (value, cls.__name__))", + 'pcapkit.vendor.http.method': "raise ValueError('%r is not a valid %s' % (value, cls.__name__))", + 'pcapkit.vendor.reg.apptype.apptype': "raise ValueError(f'{{value!r}} is not a valid {{cls.__name__}}')", +} class _StdIntEnum(enum.IntEnum): @@ -577,7 +589,9 @@ class ConstEnumGuardTemplateTests(unittest.TestCase): agree with it -- the next crawl would simply revert them. The four templates below each carry their own copy of the guard rather than inheriting the one in :mod:`pcapkit.vendor.default`, which is why all four had to be edited and - why all four are checked. + why all four are checked -- each against its own guard text now that + GitHub issue #792 moved one of them off ``%`` formatting, per + :data:`BESPOKE_TEMPLATES`. """ def setUp(self) -> None: @@ -586,13 +600,12 @@ def setUp(self) -> None: @unittest.skipUnless(importlib.util.find_spec('requests') is not None, 'pcapkit.vendor needs requests') def test_every_bespoke_template_carries_the_guard(self) -> None: - for module_name in BESPOKE_TEMPLATES: + for module_name, guard in BESPOKE_TEMPLATES.items(): with self.subTest(vendor=module_name): source = inspect.getsource(importlib.import_module(module_name)) self.assertIn( - "raise ValueError('%r is not a valid %s' % (value, cls.__name__))", - source, f'{module_name} no longer emits the guard; ' - f'see GitHub issue #647') + guard, source, f'{module_name} no longer emits its guard; ' + f'see GitHub issue #647') @unittest.skipUnless(importlib.util.find_spec('requests') is not None, 'pcapkit.vendor needs requests')