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()