Skip to content

docs(contributing): correct four statements the recent merges falsified - #656

Merged
JarryShaw merged 1 commit into
mainfrom
docs/contributing-ci-lint-corrections
Sep 22, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
docs/contributing-ci-lint-corrections

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of this pull request?

  • docs — documentation only

CONTRIBUTING.md had accumulated four statements that the tree no longer supports. One was the
reported defect; three came out of checking the rest of the file against da381f259 while in there.

1. The reported defect — "not run by the pull-request workflows"

CONTRIBUTING.md:104 read:

None of these is run by the pull-request workflows, so they are a local gate rather than something a
pull request will fail on. Running them anyway saves a review round.

#626 made that false when it added .github/workflows/lint.yml. The correction states, from the
workflow rather than from memory:

linter in lint.yml? triggers blocking?
bandit yes pull_requestmain, schedule Sat 06:00 UTC, workflow_dispatch no — continue-on-error: true
mypy yes same no — continue-on-error: true
vermin yes (vermin-ci) same no — continue-on-error: true
pylint yes same no — continue-on-error: true
isort no only cron-vendor.yml, as a formatter n/a

Three things the new prose is careful about, because getting any of them wrong would mislead a
contributor in a new direction:

  • Advisory, not blocking. All four steps carry continue-on-error: true
    (lint.yml:117,134,163,183), so a red linter cannot fail a pull request. "Linting runs in CI"
    without that qualifier would be worse than the stale sentence it replaces.
  • isort is excluded. It is not in lint.yml at all; its only workflow appearance is inside
    cron-vendor.yml, re-sorting regenerated constants as a formatter rather than checking anything.
    Import ordering is still unverified on a pull request.
  • make vermin does not fail on violations. Makefile:147-150 redirects into temp/vermin.txt
    and hands the file to code, falling back to cat, so the status returned is the viewer's.
    make vermin-ci (Makefile:158-159) is the target CI actually runs.

The text also records that CI invokes the same Makefile targets with RUN=, which is what keeps
local and CI from drifting, and points at the workflow header for the current counts rather than
copying numbers that will go stale.

2. Three further corrections

  • CONTRIBUTING.md:11 claimed the README has a Testing section holding the test commands. It
    does not — the README's headings are Installation, Usage and Documentation, and its only
    mention of testing is a row in the Documentation table (README.md:97) linking to
    docs/source/testing.rst, which is where those commands moved.
  • CONTRIBUTING.md:76 said the Changelog drift job runs "on every push and pull request". The
    job carries no if: guard, but its workflow's triggers are push: branches: [main] and
    pull_request: branches: [main], so it is scoped to main.
  • CONTRIBUTING.md:86 said the Markdown exception "stops at the root". The three issue and
    pull-request templates under .github/ are Markdown for exactly the GitHub-rendering reason the
    root files are; the claim that actually holds is about docs/source/, which has no .md files.

3. Templates — assessed, and deliberately unchanged

The second half of the ask was whether the issue/PR templates need additions, including whether new
templates are warranted. Verdict: no change, on measured evidence rather than taste.

This deliberately does not re-litigate #637, which added bug_report.md's Reproduction section,
expanded the PR template to the eight commit types at CONTRIBUTING.md:133, and added config.yml.
Its reasoning — that the template audience is the external newcomer, so the templates should not be
rewritten to imitate maintainer prose — is the reasoning this assessment follows.

Recommended but not done, as judgement calls for the owner rather than edits to make unilaterally:

  1. A PR-template checkbox for "changes wire output or public API". fix(tcp): seed TCP.read's flag accumulator with Enum_Flags(0), not a no-op cast (#616) #634, fix(pcap): Frame.len is the on-wire length, cap_len the captured one (#618) #635 and fix(protocol): refuse a construction keyword no signature declares (#617) #640 all needed
    that flagged by hand. It is cheap — one line in an existing checklist, no chooser cost — but all
    three are maintainer-authored, and the maintainer uses the PR template in 0 of the last 20 PRs,
    so the box would be seen mainly by the 4-external-PR audience that does not make such changes.
  2. PULL_REQUEST_TEMPLATE.md:8 names make pylint, make mypy, make isort and omits bandit
    and vermin. Not false — it reads as examples — but it is now a subset of what CI reports.

Notes

  • No changelog entry, deliberately. None of this is user-visible, and a bullet here would collide
    with the changelog consolidation in flight across several PRs.
  • .github/workflows/** was read-only throughout; no workflow was touched ahead of the 1.5.0b5
    release. No file outside CONTRIBUTING.md is modified.
  • The test suite was not run — nothing here executes code. config.yml was validated by parsing it.
  • One commit, authored Jarry Shaw <jarryshaw@icloud.com>, on top of da381f259.

The *Coding style* section still told contributors that none of the linters is
run by the pull-request workflows, which #626 made false when it added
`.github/workflows/lint.yml`. Saying only that they now run would mislead in the
other direction, so the replacement leads with the part that decides what a
contributor should expect: all four steps carry `continue-on-error: true`, so a
red linter cannot fail a pull request. It also keeps `isort` out of that group --
isort appears in `cron-vendor.yml` as a formatter over the regenerated
constants, not as a check -- and records the `make vermin` trap, whose exit
status is the viewer's rather than vermin's, which is why `make vermin-ci` is
the target CI runs.

Three smaller corrections found while checking the rest of the file against the
tree:

* The README has no *Testing* section any more. Its only mention of testing is
  a row in the *Documentation* table linking to `docs/source/testing.rst`,
  which is where the test commands moved.
* The `Changelog drift` job's triggers are scoped to `main` -- `push` and
  `pull_request` both carry `branches: [main]` -- rather than firing on every
  push and pull request in the repository.
* "The exception stops at the root" overstated the Markdown boundary: the three
  issue and pull-request templates under `.github/` are Markdown for the same
  GitHub-rendering reason the root files are.

No changelog entry, deliberately: none of this is user-visible, and a bullet
here would collide with the changelog consolidation currently in flight.
@JarryShaw
JarryShaw merged commit 7b0df4a into main Sep 22, 2026
12 of 25 checks passed
@JarryShaw
JarryShaw deleted the docs/contributing-ci-lint-corrections branch September 22, 2026 16:10
@JarryShaw JarryShaw added the docs Pull requests that change documentation only (docs: subject prefix) label Sep 22, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict (Sonnet, independent of the model that wrote this PR)

NEEDS CHANGES

Posting this after the merge because the review was still running when this landed. All of it is
addressed in #659. Recording it here so the reasoning sits next to the change it was about.

The review was briefed to falsify rather than bless, and to give a verdict per claim with evidence it
obtained itself. It confirmed the load-bearing claims and raised three inaccuracies:

  1. CONTRIBUTING.md:108 contradicted CONTRIBUTING.md:124 four lines apart. "The job invokes the
    same Makefile targets listed above" is false for vermin — lint.yml:166 runs make vermin-ci,
    which is not in the block above — and the later paragraph said so. Fixed in docs(contributing): the make vermin trap is the opposite of what was recorded #659.
  2. "every step writes its count there" is wrong for vermin. lint.yml:173 emits version verdicts
    (Minimum required versions: / Target versions not met:), not counts; the reviewer checked this
    against vermin's own source strings in vermin/main.py:138,156,164. Fixed in docs(contributing): the make vermin trap is the opposite of what was recorded #659.
  3. Changelog drift has a trigger this PR did not mention. The job carries no gate-only guard,
    so it also runs on every workflow_call into unit-tests.yml. Fixed in docs(contributing): the make vermin trap is the opposite of what was recorded #659, naming all four
    callers.

Confirmed by the review, with its own evidence: all four continue-on-error: true lines
(lint.yml:117,134,163,183), so the advisory claim holds; the trigger set and 0 6 * * 6 decoded to
Saturday 06:00 UTC with push genuinely absent; Python 3.14 single-version; isort absent from
lint.yml and only ever a bare rewrite in cron-vendor.yml, never a check; the README having no
Testing heading; the .github/ Markdown inventory, with a repo-wide sweep finding no other .md
anywhere; and the eight commit types, the quoted example commit, and the 120/100 line lengths.

It also noted a scope point worth keeping: checkout, setup-python and the install step carry no
continue-on-error, so the job can still fail on infrastructure. The prose says "a red linter will
not fail your pull request", which is scoped to findings, so it does not overclaim — but the job going
red is not always a linter talking.

Where the cross-review was itself wrong

It marked the make vermin exit-status claim CONFIRMED — agreeing with this PR that a run finding
violations "still looks like a success", on the grounds that A && B || C always returns the viewer's
status. That is the same error the Makefile and lint.yml comments make: it reasons about that line
in isolation and misses that the redirect is a separate recipe line, so make abandons the target
before reaching it.
Measured rather than reasoned: make vermin RUN= exits 2 with
*** [vermin] Error 1, vermin exits 1 on Target versions not met: 3.6, and the viewer line never
runs — with a passing control that does reach it. #659 has the full evidence.

Two reviewers agreeing is not the same as the claim being true, which is the useful lesson here.

JarryShaw added a commit that referenced this pull request Sep 22, 2026
…s recorded (#659)

#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant