diff --git a/.github/workflows/formatting.yaml b/.github/workflows/formatting.yaml index 765c523..e473ee3 100644 --- a/.github/workflows/formatting.yaml +++ b/.github/workflows/formatting.yaml @@ -27,5 +27,22 @@ jobs: cache: true cache-dependency-path: go.sum + # `gofmt -l` walks the filesystem rather than Go's package wildcard, so + # unlike `go vet ./...` it has to be told which files are the module's own. + # scripts/go_files.sh is the one place that answers, here and locally, by + # asking the toolchain — which is what keeps vendor/ and testdata/ out. + # + # pipefail matters more than usual: without it, go_files.sh failing + # mid-pipeline is masked by xargs exiting 0 on no input, and the step + # passes having checked nothing at all. - name: check formatting - run: if [ $(gofmt -l . | grep -Ev '^vendor\/' | head -c1 | wc -c) -ne 0 ]; then exit 1; fi + run: | + set -euo pipefail + + unformatted=$(scripts/go_files.sh . | xargs -0 gofmt -l) + + if [ -n "${unformatted}" ]; then + echo "::error::gofmt reports these files as unformatted:" + echo "${unformatted}" + exit 1 + fi diff --git a/CLAUDE.md b/CLAUDE.md index e406e42..9dfe0d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,6 +55,20 @@ This template does **not** vendor dependencies (platform-go's dependency tree is tests run against the module cache. Vendoring targets (`make vendor` / `make revendor`) exist for consumers who want them. +`scripts/go_files.sh` is the one place that decides which Go files the formatters see, and +`format_golang.sh`, `format_imports.sh`, `goimports.sh`, and the `gofmt` check in +`.github/workflows/formatting.yaml` all take their list from it. It asks the Go toolchain +(`go list -e -f '{{.Dir}}' ./...`, then `find -maxdepth 1`) rather than writing out an exclusion +list, so `vendor/`, `testdata/`, and any `_`- or `.`-prefixed directory are skipped for the same +reason `go test ./...` skips them. Point new filesystem-walking tooling at it rather than adding a +fourth spelling of the same exclusion. + +Two things about it are load-bearing. It **fails loudly rather than emitting an empty list** — an +out-of-sync `vendor/modules.txt` makes `go list` exit non-zero, and a formatter that quietly formats +nothing (or a CI check that quietly checks nothing) is worse than a stop. And its callers read it +**through a file, not `< <(...)`**, because process substitution discards the exit status of what it +runs, which is exactly how that empty list would go unnoticed. + ## Import Ordering Import ordering uses `gci` with four sections, separated by blank lines: diff --git a/scripts/format_golang.sh b/scripts/format_golang.sh index cf51bb9..dde80b8 100755 --- a/scripts/format_golang.sh +++ b/scripts/format_golang.sh @@ -3,11 +3,22 @@ set -euo pipefail # Format all Go files using gofmt # Usage: format_golang.sh +# +# Which files those are is go_files.sh's question, not this one's. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_ROOT="${1:-$(pwd)}" +# Through a file rather than `< <(go_files.sh)`: process substitution discards +# the exit status of what it runs, so a failure to produce the list would read +# here as a list of no files, and formatting nothing would look like success. +file_list="$(mktemp)" +trap 'rm -f "${file_list}"' EXIT + +"${SCRIPT_DIR}/go_files.sh" "${PROJECT_ROOT}" >"${file_list}" + while IFS= read -r -d '' file; do # GO_FORMAT contains a command with arguments, so we use eval # shellcheck disable=SC2086 eval "gofmt -s -w \"${file}\"" -done < <(find "${PROJECT_ROOT}" -type f -not -path '*/vendor/*' -name "*.go" -print0) +done <"${file_list}" diff --git a/scripts/format_imports.sh b/scripts/format_imports.sh index 8d4c518..dbdb31c 100755 --- a/scripts/format_imports.sh +++ b/scripts/format_imports.sh @@ -3,15 +3,26 @@ set -euo pipefail # Format Go imports using gci # Usage: format_imports.sh +# +# Which files those are is go_files.sh's question, not this one's. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PACKAGE_PREFIX="${1:-github.com/primandproper/template-go}" PROJECT_ROOT="${2:-$(pwd)}" -# Find all Go files and pass them to gci +# Through a file rather than `< <(go_files.sh)`: process substitution discards +# the exit status of what it runs, so a failure to produce the list would read +# here as a list of no files, and formatting nothing would look like success. +file_list="$(mktemp)" +trap 'rm -f "${file_list}"' EXIT + +"${SCRIPT_DIR}/go_files.sh" "${PROJECT_ROOT}" >"${file_list}" + +# Every Go file the module owns, passed to gci go_files=() while IFS= read -r -d '' file; do go_files+=("${file}") -done < <(find "${PROJECT_ROOT}" -type f -not -path '*/vendor/*' -name "*.go" -print0) +done <"${file_list}" if [ ${#go_files[@]} -gt 0 ]; then go tool gci write \ diff --git a/scripts/go_files.sh b/scripts/go_files.sh new file mode 100755 index 0000000..5d3d99f --- /dev/null +++ b/scripts/go_files.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Emit every Go file this module owns, NUL-separated, for the formatters to +# consume with `read -r -d ''`. +# Usage: go_files.sh [project_root] +# +# The list comes from the Go toolchain rather than from a find(1) exclusion +# list, because the toolchain already knows the answer. `./...` does not +# descend into vendor/ or testdata/, nor into any directory whose name begins +# with _ or . — which is why every wildcard-driven target here (test, lint, +# go fix, fieldalignment, tagalign) needs no exclusions at all, and only the +# tools that walk the filesystem ever did. +# +# Those tools used to answer the question themselves, three different ways: +# format_golang.sh and format_imports.sh each carried their own +# `-not -path '*/vendor/*'`, the formatting workflow filtered gofmt's output +# with `grep -Ev '^vendor\/'`, and goimports.sh ran `goimports -w .` with no +# exclusion whatsoever — which rewrote every vendored file in the tree. One +# question deserves one answer, and this is it. +# +# -e keeps a package that does not compile in the list. Formatting a file is +# most useful exactly when it is still broken, so a syntax error somewhere in +# the module must not empty the whole list. +# +# -maxdepth 1 because what go list names are package directories, and a package +# is entitled to a testdata directory of its own. + +PROJECT_ROOT="${1:-$(pwd)}" + +cd "${PROJECT_ROOT}" + +# go list is asked for the directories separately from walking them, so that a +# failure to list is a failure of this script rather than an empty answer. +# `go list` exits non-zero for module-level problems that -e does not cover — an +# out-of-sync vendor/modules.txt being the one to expect — and a formatter that +# quietly formats nothing, or a CI check that quietly checks nothing, is a worse +# outcome than either a wrong file list or a loud stop. +package_dirs="$(go list -e -f '{{.Dir}}' ./...)" + +if [ -z "${package_dirs}" ]; then + echo "go_files.sh: go list named no packages under ${PROJECT_ROOT}" >&2 + exit 1 +fi + +printf '%s\n' "${package_dirs}" | while IFS= read -r dir; do + # A package with no directory on disk is not something to walk; -e means the + # list can carry entries the loader could not resolve. + [ -n "${dir}" ] && [ -d "${dir}" ] || continue + + find "${dir}" -maxdepth 1 -type f -name '*.go' -print0 +done diff --git a/scripts/goimports.sh b/scripts/goimports.sh index 4555c47..d99f3ab 100755 --- a/scripts/goimports.sh +++ b/scripts/goimports.sh @@ -2,6 +2,29 @@ set -euo pipefail # Format Go imports using goimports -# Usage: goimports.sh +# Usage: goimports.sh [project_root] +# +# This used to be `go tool goimports -w .`, which walks the filesystem from the +# working directory and so rewrote every vendored file in the tree whenever one +# was present. It takes its file list from go_files.sh now, like the other +# formatters. -go tool goimports -w . +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="${1:-$(pwd)}" + +# Through a file rather than `< <(go_files.sh)`: process substitution discards +# the exit status of what it runs, so a failure to produce the list would read +# here as a list of no files, and formatting nothing would look like success. +file_list="$(mktemp)" +trap 'rm -f "${file_list}"' EXIT + +"${SCRIPT_DIR}/go_files.sh" "${PROJECT_ROOT}" >"${file_list}" + +go_files=() +while IFS= read -r -d '' file; do + go_files+=("${file}") +done <"${file_list}" + +if [ ${#go_files[@]} -gt 0 ]; then + go tool goimports -w "${go_files[@]}" +fi