From fd732a18ee499b9a86579f385d3969a1b153206f Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 24 Sep 2026 20:04:01 -0400 Subject: [PATCH] ci(lint): make all three isort lines clean and repin lint.yml's counts - schema.py's `warnings` import was still wrapped from an earlier, narrower width; under the Makefile's real isort flags (-l100 -ppcapkit) it now fits on one line. dispatch.py needed a blank line after two function-local import blocks; options.py needed `import ipaddress` moved above its `pcapkit...` block and a `scapy.all` import reordered -- import order only, confirmed via ast/py_compile and the targeted test batch that exercises both files. All three lines of the Makefile's `isort:` target are now clean; none needed a config change (isort's 100 vs pylint's 120 is a documented, deliberate split per CONTRIBUTING.md, not a misconfiguration). - tests/project/test_isort_clean.py now runs all three Makefile lines, each as its own subTest, rather than just the one reaching schema.py; skips when isort is not installed, the same precondition the Makefile target has. Its note on why options.py's reorder is safe now cites three test modules that actually load it by path plus dispatch.py:20 and the 1.5.0 changelog, replacing a `CONTRIBUTING.md:110` citation that turned out not to exist. - Re-measured all four analysers in lint.yml's header at 932cb48d1 and pinned each line to that commit (#753): bandit unchanged (8), mypy 112/38 (was 115/39), vermin 106 files (was 105 -- already 106 at 73f09aecb too, so three of the four had drifted by the time #753 was filed, not two). pylint's E/W/C (90/4765/542, file-scoped -- both E and W exclude one Command-line-attributed message each, matching pylint's own `messageTypeCount`) are pinned; its R and total are not, since repeated runs at the same commit gave R 645-680 (all from non-deterministic `R0401` cyclic-import ordering) -- documented instead of pinned. Five of `PYLINT_FLAGS`' entries are stale against the installed pylint, not three; the fifth (`pylint.extensions.emptystring`, a dead plugin) is #767 and out of scope here. The narrowed `--enable=E,F` figure in the "Promotion order" prose is corrected to a real, file-scoped run of that invocation (87 errors, 36 unsubscriptable-object, 31 no-member). Also fixed the unwrapped `warnings` import's character count in the test docstring (96, not 98). Build/test: all three `make isort` lines clean; new test passes and is shown to fail on each of its three subTests independently when its fix is reverted; tests/project/ 127 passed, 489 subtests, under `coverage run -m pytest`. --- .github/workflows/lint.yml | 53 +++++++--- examples/generators/dispatch.py | 2 + examples/generators/options.py | 5 +- pcapkit/protocols/schema/schema.py | 3 +- tests/project/test_isort_clean.py | 152 +++++++++++++++++++++++++++++ 5 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 tests/project/test_isort_clean.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5f559d57d..41f767ef1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -7,13 +7,44 @@ name: "Lint" # is where they run. # # Every one of them is advisory -- `continue-on-error: true` on each step -- and -# that is a measurement result rather than a preference. Counts on 15189abff, -# each from the same flag sets the Makefile uses: +# that is a measurement result rather than a preference. Each line below is +# pinned to the commit it was taken at, rather than left as a bare count: #753 +# was filed when three of the four had already drifted from the figures this +# comment once recorded (on 15189abff) -- mypy and pylint by the issue's own +# numbers, and vermin too (105 recorded; 106 measured here both at 73f09aecb, +# where #753 was filed, and at 932cb48d1 below, so it had already moved before +# the issue's own text said it "was not re-checked"). Only bandit held. A bare +# number invites exactly this, with no way to tell staleness from disagreement. +# All four happen to be re-measured together below, each from the same flag +# sets the Makefile uses; re-measuring only one tool later should update only +# that line's commit. # -# bandit 8 findings (7 medium, 1 low, 0 high) exit 1 -# mypy 115 errors in 39 files (496 checked) exit 1 -# vermin 105 files flagged; needs 3.11, targets 3.6 in vermin.ini exit 1 -# pylint 5902 messages (80 E, 4767 W, 519 R, 541 C) exit 30 +# bandit 8 findings (7 medium, 1 low, 0 high) exit 1 [932cb48d1] +# mypy 112 errors in 38 files (506 checked) exit 1 [932cb48d1] +# vermin 106 files flagged; needs 3.11, targets 3.6 in vermin.ini exit 1 [932cb48d1] +# pylint 90 E, 4765 W, 542 C -- stable; R and the total are not, see below exit 30 [932cb48d1] +# +# pylint's R and total are deliberately not pinned to a single number: repeated +# runs on the identical tree at 932cb48d1 gave R of 645-680 and totals of +# 6044-6079 (two runs measured here: 659/6058 and 680/6079; a cross-review +# measured four more: 662/647/647/645, totals 6061/6046/6046/6044), entirely +# from `R0401` cyclic-import, whose count depends on module-processing order +# rather than on the code -- `duplicate-code` (400) and every other R check +# held steady across all of them. Separately, the job's own grep-based count +# (the `pylint` step below) and this header can legitimately disagree on the +# *same* run: four of `PYLINT_FLAGS`' `--disable=` entries name checks pylint +# has since removed or renamed (`old-division`, `no-absolute-import`, +# `input-builtin` -> `R0022 useless-option-value`; `eq-without-hash` -> +# `W0012 unknown-option-value`), plus one plugin in `--load-plugins=` that no +# longer exists in the installed pylint (`pylint.extensions.emptystring` -> +# `E0013 bad-plugin-value`, see #767) -- five stale flags in all, and pylint +# attributes every one of them to `Command line` rather than to a file. +# Whether a count includes them is a choice, not a fact about the tree -- C +# above is untouched either way (none of the five is class C); E and W above +# both exclude their one Command-line message each (91 and 4766 with them, 90 +# and 4765 without -- pylint's own `messageTypeCount` agrees: 90/4765/542); +# R's swing above already includes its three either way, dwarfed by +# `R0401`'s own. # # None of the four is clean, so none can block today: a job that is red the day # it lands teaches everyone to scroll past red, which costs more than the checks @@ -26,17 +57,17 @@ name: "Lint" # `address: ... = '0.0.0.0'` *default arguments* in MH/MIP option builders, # which bind no socket. This package already uses `# nosec` (16 findings are # suppressed that way today), so triaging 8 comments is the whole job. -# * mypy -- 24 of the 115 are `unused-ignore`, i.e. `# type: ignore` comments +# * mypy -- 24 of the 112 are `unused-ignore`, i.e. `# type: ignore` comments # that are no longer needed. Deleting those is risk-free and cuts a fifth. # * vermin -- blocked on a decision, not on cleanup: see the step below. -# * pylint -- not realistically promotable at this rule set. 4358 of its 4767 -# warnings are `unused-argument` (2931), `redefined-builtin` (808) and +# * pylint -- not realistically promotable at this rule set. 4356 of its 4765 +# warnings are `unused-argument` (2929), `redefined-builtin` (808) and # `super-init-not-called` (619), all three of which are inherent to the # schema DSL: protocol fields are legitimately named `next`/`type`/`id`, and # the `if TYPE_CHECKING: def __init__(...)` stubs exist precisely so as not # to call `super().__init__`. Narrowing to `--enable=E,F` does not rescue it -# either -- that is still 80 errors, mostly `unsubscriptable-object` (40) and -# `no-member` (22) false positives off the same metaprogramming. +# either -- that is still 87 errors, mostly `unsubscriptable-object` (36) and +# `no-member` (31) false positives off the same metaprogramming. on: # Deliberately *not* `push: [main]`, which is the trigger the other validation diff --git a/examples/generators/dispatch.py b/examples/generators/dispatch.py index 7c70f755e..f1b60666e 100644 --- a/examples/generators/dispatch.py +++ b/examples/generators/dispatch.py @@ -283,6 +283,7 @@ def _link_payload(code: 'int') -> 'bytes': return bytes(ARP(oper=1)) if code == EtherType.Reverse_Address_Resolution_Protocol: from pcapkit.protocols.link.rarp import RARP + # ARP/RARP/InARP/DRARP all report their alias from the wire ``oper`` # field rather than from the dispatching EtherType (link/arp.py:176-190 # per #496), so ``oper`` has to land in the RARP range (3, 4) or this @@ -378,6 +379,7 @@ def _internet_payload(code: 'int') -> 'bytes': if code == TransType.HIP: from pcapkit.const.hip.parameter import Parameter from pcapkit.protocols.internet.hip import HIP + # Two copies of one parameter, not one -- HIP.make's header length # arithmetic loses four octets for a lone parameter and the packet is # then rejected on the way back in; see HIP_COPIES in diff --git a/examples/generators/options.py b/examples/generators/options.py index 67b2a3ad2..a15847e32 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -821,11 +821,12 @@ def _mh_option_registry() -> 'Any': def _mh_option_overrides() -> 'dict[Any, dict[str, Any]]': + import ipaddress + from pcapkit.const.mh.ani_suboption import ANISuboption as Enum_ANISuboption from pcapkit.const.mh.lma_mag_suboption import \ LMAControlledMAGSuboption as Enum_LMAControlledMAGSuboption from pcapkit.const.mh.option import Option as Enum_Option - import ipaddress return { # ``{}`` makes ``_make_opt_pad`` warn and silently emit a Pad1, so the # PadN case would otherwise be a duplicate of the Pad1 one. @@ -1797,7 +1798,7 @@ def _frame(family: 'Family', octets: 'bytes', index: 'int') -> 'Any': ValueError: If ``family`` names an envelope this function does not know. """ - from scapy.all import IP, IPv6, Ether, Raw, TCP # pylint: disable=no-name-in-module + from scapy.all import IP, TCP, Ether, IPv6, Raw # pylint: disable=no-name-in-module link = Ether(src=SRC_MAC, dst=DST_MAC) if family.envelope == 'ethernet-ipv4': diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index fceca0e6a..f0a4a4a3d 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -16,8 +16,7 @@ from pcapkit.utilities.compat import Mapping from pcapkit.utilities.decorators import prepare from pcapkit.utilities.exceptions import NoDefaultValue, ProtocolUnbound, SchemaError, stacklevel -from pcapkit.utilities.warnings import (RegistryWarning, SchemaWarning, UnknownFieldWarning, - warn) +from pcapkit.utilities.warnings import RegistryWarning, SchemaWarning, UnknownFieldWarning, warn if TYPE_CHECKING: from collections import OrderedDict diff --git a/tests/project/test_isort_clean.py b/tests/project/test_isort_clean.py new file mode 100644 index 000000000..a82419507 --- /dev/null +++ b/tests/project/test_isort_clean.py @@ -0,0 +1,152 @@ +# -*- coding: utf-8 -*- +"""Pins ``make isort`` clean on all three of its lines (#757). + +#757 was filed from a reproduction that ran bare ``isort --check-only --diff +pcapkit/protocols/schema/schema.py``, with none of the flags ``make isort`` +actually uses. Bare isort wraps at its own default of 79 columns, so it flagged +the ``misc`` import (89 characters) and proposed wrapping it -- and from that +diff the issue reasoned that isort's wrap width must disagree with the +project's ``line-too-long`` threshold of 120, since 89 is under 120 but over +79. + +Re-running with the Makefile's own flags -- see :data:`MAKEFILE_LINES` below, +one entry per line of the ``isort:`` target -- shows a different picture. +Those flags set the width to 100, exactly as :file:`CONTRIBUTING.md` documents +("120 for ``pylint`` and 100 for ``isort``, not PEP 8's 79"): a deliberate, +written-down split, not a misconfiguration. Under *that* width the ``misc`` +import is not flagged at all (89 < 100). What *was* flagged, on the commit +this test landed with, were three things line 125's flags alone would not +have caught: + +* :file:`pcapkit/protocols/schema/schema.py` -- its ``pcapkit.utilities.warnings`` + import was wrapped across two lines at some earlier width, and unwrapped it + is 96 characters, under 100, so isort's real verdict was to collapse it back + to one line rather than wrap another import. +* :file:`examples/generators/dispatch.py` -- two function-local import blocks + isort wants a blank line after (line 127's targets, unreachable from line + 125's ``pcapkit`` argument). +* :file:`examples/generators/options.py` -- an ``import ipaddress`` isort wants + moved above the ``from pcapkit...`` block, and a ``from scapy.all import + ...`` isort wants reordered by its constant/class split (``IP, TCP`` before + ``Ether, IPv6, Raw``). Also line 127 only; only import order changed, since + this module is loaded and run at test time, not just read at review time. + Three test modules load it by path via + ``importlib.util.spec_from_file_location``: + :file:`tests/protocols/test_option_generator_tcp_base_unit.py`, + :file:`tests/protocols/test_option_coverage_runtime.py`, and + :file:`tests/protocols/test_option_roundtrip_unit.py`. + :file:`examples/generators/dispatch.py:20` and + :file:`docs/source/changelog/1.5.0.rst` (lines 691 and 1169) both name it + directly, too. + +So this module does not re-litigate the width. It just pins isort's own +verdict on each of the three lines -- via real ``isort`` invocations with the +Makefile's exact flags, not a hand-rolled guess about what those flags imply -- +so a future edit that reintroduces an unsorted or mis-wrapped import anywhere +``make isort`` reaches fails here instead of surfacing as "clean locally, red +in CI" the way #757 did. Earlier revisions of this test covered line 125 +only (:file:`pcapkit`, skipping ``__init__.py``) and so missed exactly the +:file:`examples/generators/` files above and the ``__init__.py`` files that +sit under :file:`pcapkit/const/*/` or :file:`pcapkit/vendor/*/` -- line 126 has +no ``--skip-glob``, so it re-covers whichever of the 71 files line 125 skips +happen to live there, even though line 125 skips all 71 package-wide. + +``isort`` is deliberately absent from both ``Pipfile`` and ``pyproject.toml``: +:file:`.github/workflows/lint.yml` notes it stays local-only, unlike the other +three linters. So this test skips outright when isort is not installed, the +same way :class:`tests.project.test_release_gates.TestYAMLAgreesWithTheScanner` +skips its PyYAML-dependent half. #766 covers why that skip is itself invisible +to :file:`tests/_dependency_gates.py`'s guard -- deferred there, not fixed here. + +""" + +from __future__ import annotations + +import pathlib +import subprocess +import sys +import unittest + +ROOT = pathlib.Path(__file__).resolve().parents[2] +PACKAGE = ROOT / 'pcapkit' + + +def _line_125_targets() -> 'list[str]': + """``pcapkit $(wildcard temp/sort.py)`` -- the scratch file is untracked + and normally absent, so this mirrors make's ``$(wildcard ...)`` by only + adding it when it actually exists. + + """ + targets = [str(PACKAGE)] + scratch = ROOT / 'temp' / 'sort.py' + if scratch.exists(): + targets.append(str(scratch)) + return targets + + +def _line_126_targets() -> 'list[str]': + """``pcapkit/{const,vendor}/*/*.py``, expanded the way bash would.""" + targets = [] # type: list[str] + for sub in ('const', 'vendor'): + targets.extend(sorted(str(path) for path in (PACKAGE / sub).glob('*/*.py'))) + return targets + + +def _line_127_targets() -> 'list[str]': + """``util/*.py examples/generators/*.py``, expanded the way bash would.""" + targets = sorted(str(path) for path in (ROOT / 'util').glob('*.py')) + targets.extend(sorted(str(path) for path in (ROOT / 'examples' / 'generators').glob('*.py'))) + return targets + + +#: One entry per line of the Makefile's ``isort:`` target: the flags that +#: precede the targets, and the callable that resolves those targets on this +#: checkout. Kept identical to those three lines on purpose -- a flag changed +#: in one place and not the other is exactly how "clean locally, red in CI" +#: (or the reverse) starts. +MAKEFILE_LINES = { + 125: (['-l100', '-ppcapkit', '--skip-glob', '**/__init__.py'], _line_125_targets), + 126: (['-l100', '-ppcapkit'], _line_126_targets), + 127: (['-l100', '-ppcapkit'], _line_127_targets), +} + + +class TestIsortIsCleanOnThePackage(unittest.TestCase): + """``make isort`` must not be red on a clean checkout, on any of its three + lines (#757). + + """ + + @classmethod + def setUpClass(cls) -> None: + try: + import isort # noqa: F401 pylint: disable=unused-import,import-outside-toplevel + except ImportError: + raise unittest.SkipTest( + "isort is not installed -- it is deliberately absent from both " + "Pipfile and pyproject.toml (lint.yml: 'still local-only'), so " + "this test only runs when a contributor has it installed, the " + "same precondition `make isort` itself has" + ) + + def test_check_only_is_clean_on_every_makefile_line(self) -> None: + """Each line of the Makefile's ``isort:`` target, run the way it runs.""" + for line, (flags, targets_fn) in sorted(MAKEFILE_LINES.items()): + with self.subTest(makefile_line=line): + targets = targets_fn() + self.assertTrue(targets, f'Makefile:{line} resolved to no targets on this ' + f'checkout -- the glob is broken, not clean') + + result = subprocess.run( + [sys.executable, '-m', 'isort', '--check-only', *flags, *targets], + cwd=ROOT, capture_output=True, text=True, check=False, + ) + self.assertEqual( + result.returncode, 0, + f'`make isort` is red on a clean checkout -- Makefile:{line} wants ' + f'changes:\n{result.stdout}{result.stderr}', + ) + + +if __name__ == '__main__': + unittest.main()