From 28a3a9f181d832ac5238e75e113e336b724fafec Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 12:19:18 -0400 Subject: [PATCH] docs(contributing): the `make vermin` trap is the opposite of what was recorded #656 said `make vermin`'s exit status is the viewer's, so a run that found violations still looks like a success. That is what `Makefile:152-157` and `lint.yml:150-152` both assert, and it is wrong: the redirect into `temp/` is its own recipe line, so make gives up there and never reaches the line that would open the report. Measured on this tree -- `make vermin RUN=` exits 2 with `*** [vermin] Error 1`, vermin itself exits 1 on `Target versions not met: 3.6` against a real floor of 3.11, and the 365-line report is left unread in `temp/vermin.txt`. A control run with a succeeding command does reach the viewer line, which is what makes the redirect the deciding factor rather than the shell semantics of `A && B || C`. So the trap is real but inverted: the failure propagates and the *findings* are what get hidden. A contributor told the old version would ignore a red `make vermin` as a known false pass. Three precision fixes in the same section, from a cross-review of #656: * The CI paragraph claimed the job runs "the same `Makefile` targets listed above". It runs `vermin-ci` for vermin, which is not one of them, and the paragraph four lines later said so -- the file contradicted itself. It now names which target each tool takes, and rests the no-drift claim on the shared flag variables, which is what actually holds. * "every step writes its count" was wrong for vermin, whose summary line is a version verdict rather than a count. * `Changelog drift` also runs on every `workflow_call` into `unit-tests.yml` -- the vendor and conda updates, the pages deploy and the release all gate on it -- because that job carries no `gate-only` guard. The two stale comments in `Makefile` and `lint.yml` are deliberately left alone: a workflow edit ahead of the 1.5.0b5 release is not worth the risk, and they want their own change. No changelog entry, deliberately: none of this is user-visible, and a bullet here would collide with the changelog consolidation currently in flight. --- CONTRIBUTING.md | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5cf911a4f..0c2ab54c4 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,7 +74,9 @@ python util/changelog_md.py --check # exits 0 when they agree, prints a diff The generator needs only the standard library, so it runs against a bare interpreter. The `Changelog drift` job in `.github/workflows/unit-tests.yml` runs `--check` on every push to `main` -and every pull request targeting it, and a hand-edited `CHANGELOG.md` will fail it. +and every pull request targeting it, and again whenever another workflow calls that one as a gate — +the vendor and conda updates, the pages deploy and the release all do, and the job carries no +`gate-only` guard to opt out. A hand-edited `CHANGELOG.md` will fail it. ## Documentation @@ -105,24 +107,28 @@ make vermin # minimum-Python-version check Four of them — `pylint`, `mypy`, `bandit` and `vermin` — also run in CI, as the `Lint` job in `.github/workflows/lint.yml`: on every pull request against `main`, on a weekly Saturday schedule and on demand through `workflow_dispatch`, on Python 3.14 alone rather than across the test matrix. -The job invokes the same `Makefile` targets listed above, with `RUN=` emptying the `pipenv run` -prefix, so what CI checks and what you check locally cannot drift apart. +The job drives them through the `Makefile` rather than restating their flags — `pylint`, `mypy` and +`bandit` by the targets above, `vermin` by its `vermin-ci` variant — with `RUN=` emptying the +`pipenv run` prefix. Both paths read the same flag variables, so a check cannot come out clean +locally and red in CI because the two definitions drifted. **Those steps are advisory, not a gate.** Each carries `continue-on-error: true`, so a finding lands as a non-blocking annotation and a red linter will not fail your pull request. That is a consequence of none of the four being clean today; the workflow's header records the current counts and what each tool would need before its `continue-on-error` line could be deleted. Read the job's run -summary — every step writes its count there — and treat it as information you should not add to. +summary — each step writes its own verdict there — and try not to add to the numbers. `isort` is the exception and is still local-only. It does appear in `cron-vendor.yml`, but as a formatter that rewrites the regenerated constants rather than as a check, so nothing verifies import ordering on a pull request. -One trap in the target list above: `make vermin` writes its report to `temp/vermin.txt` and hands -that file to a viewer — VS Code if it is on `PATH`, otherwise `cat`. The status you get back is -that viewer's rather than vermin's, so a run that found violations still looks like a success. -`make vermin-ci` is the same check without the redirect, and it is the one CI runs. Running the lot -before you push still saves a review round. +One trap in the target list above: `make vermin` redirects its report into `temp/vermin.txt` +rather than to your terminal, and vermin exits 1 today because `vermin.ini` sets `targets = 3.6` +against a real floor of 3.11. Make gives up at that redirect, so the run fails with +`make: *** [vermin] Error 1`, the report left in the file, and the line that would have opened it +never reached. `make vermin-ci` runs the same flags straight to stdout, which is why CI uses it +and why it is the easier of the two to read at a desk. Running the lot before you push still saves +a review round. ### Format of the Commit Message