ci(lint): make all three isort lines clean and repin lint.yml's counts

.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

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

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':

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

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