From dad57b70d09469c68ec6af9f57c24f0f88c0b124 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 29 Sep 2026 09:52:47 -0300 Subject: [PATCH] allow: the ratchet is split in one place internal/allow says it exists so refs and testquality read their lists alike, and each still split its findings on its own: refs behind an Allow type that held a map and prefixed an error, testquality inline. Split is allow's now, generic over the finding and its key, and refs.Allow is gone. The why of the stale half moves with it. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/refs/main.go | 9 +++---- cmd/testquality/main.go | 21 ++++++---------- internal/allow/allow.go | 28 +++++++++++++++------- internal/allow/allow_test.go | 14 +++++++---- internal/refs/allow.go | 46 ------------------------------------ internal/refs/refs.go | 4 +++- internal/refs/refs_test.go | 8 +++---- 7 files changed, 48 insertions(+), 82 deletions(-) delete mode 100644 internal/refs/allow.go diff --git a/cmd/refs/main.go b/cmd/refs/main.go index 44005f7..7cfa91b 100644 --- a/cmd/refs/main.go +++ b/cmd/refs/main.go @@ -16,6 +16,7 @@ import ( "slices" "strings" + "github.com/spin-stack/go-tools/internal/allow" "github.com/spin-stack/go-tools/internal/refs" ) @@ -40,9 +41,9 @@ func taskExeFromEnv() string { } func run(root, allowPath string, pathspecs []string, taskExe string) error { - allow, err := refs.ReadAllow(allowPath) + allowed, err := allow.Read(allowPath) if err != nil { - return err + return fmt.Errorf("refs: %w", err) } // A document either has something holding it to the tree or it is deleted, and this is // what holds the ones named. @@ -72,10 +73,10 @@ func run(root, allowPath string, pathspecs []string, taskExe string) error { if err != nil { return err } - unexplained, stale := allow.Split(found) + unexplained, stale := allow.Split(allowed, found, refs.Finding.Key) fmt.Printf("scanned: %d files references that do not resolve: %d explained (%s): %d\n", - len(files), len(found), allowPath, allow.Len()) + len(files), len(found), allowPath, len(allowed)) if len(stale) > 0 { fmt.Fprint(os.Stderr, "\nstale entries in the allowlist - nothing matches these any more, so the sentence\n"+ diff --git a/cmd/testquality/main.go b/cmd/testquality/main.go index 8a225ee..89d9f83 100644 --- a/cmd/testquality/main.go +++ b/cmd/testquality/main.go @@ -35,22 +35,15 @@ func main() { os.Exit(2) } - var unexplained []string - used := map[string]bool{} - for _, f := range findings { - if allowed[f.Key()] { - used[f.Key()] = true - continue - } - unexplained = append(unexplained, fmt.Sprintf("%s:%d: %s", relative(f.Pos.Filename), f.Pos.Line, f.Message)) + unexplained, stale := allow.Split(allowed, findings, testquality.Finding.Key) + lines := make([]string, 0, len(unexplained)) + for _, f := range unexplained { + lines = append(lines, fmt.Sprintf("%s:%d: %s", relative(f.Pos.Filename), f.Pos.Line, f.Message)) } + sort.Strings(lines) - // Stale exemptions fail too, or the list stops shrinking. - stale := allow.Stale(allowed, used) - sort.Strings(unexplained) - - for _, u := range unexplained { - fmt.Fprintln(os.Stderr, u) + for _, l := range lines { + fmt.Fprintln(os.Stderr, l) } for _, s := range stale { fmt.Fprintf(os.Stderr, "%s is in %s and is no longer a finding: remove the line\n", s, *allowPath) diff --git a/internal/allow/allow.go b/internal/allow/allow.go index 9bf347a..024fa68 100644 --- a/internal/allow/allow.go +++ b/internal/allow/allow.go @@ -43,15 +43,27 @@ func Read(path string) (map[string]bool, error) { return allowed, nil } -// Stale is the keys of allowed that matched does not hold, sorted: an entry that excuses nothing -// is a sentence rewritten or a test fixed, and a list that keeps it stops being a ratchet. -func Stale(allowed, matched map[string]bool) []string { - var stale []string - for key := range allowed { - if !matched[key] { - stale = append(stale, key) +// Split turns findings into the two halves the ratchet reports: what nothing in allowed explains, +// in the order found, and the keys of allowed that explain nothing, sorted. +// +// The second half is the direction people forget. An entry that matches nothing means the +// sentence was rewritten or the test fixed, and a list that keeps it goes on excusing something +// nobody does - which is how an allowlist stops being a ratchet and becomes a place to put things. +func Split[T any](allowed map[string]bool, found []T, key func(T) string) (unexplained []T, stale []string) { + matched := map[string]bool{} + for _, f := range found { + k := key(f) + if allowed[k] { + matched[k] = true + continue + } + unexplained = append(unexplained, f) + } + for k := range allowed { + if !matched[k] { + stale = append(stale, k) } } sort.Strings(stale) - return stale + return unexplained, stale } diff --git a/internal/allow/allow_test.go b/internal/allow/allow_test.go index 3df1bce..c0d2873 100644 --- a/internal/allow/allow_test.go +++ b/internal/allow/allow_test.go @@ -50,9 +50,15 @@ func TestAnExceptionNeedsItsReasonAndItsFile(t *testing.T) { } } -func TestStaleIsWhatNothingMatched(t *testing.T) { - got := allow.Stale(map[string]bool{"b": true, "a": true, "c": true}, map[string]bool{"c": true}) - if !slices.Equal(got, []string{"a", "b"}) { - t.Errorf("stale = %q", got) +// Both halves of the ratchet: a finding nothing explains keeps its place, and an entry that +// explains nothing is named, however many findings it would have explained. +func TestSplitIsWhatNothingExplainsAndWhatExplainsNothing(t *testing.T) { + allowed := map[string]bool{"b": true, "a": true, "c": true} + unexplained, stale := allow.Split(allowed, []string{"z", "c", "y", "c"}, strings.ToLower) + if !slices.Equal(unexplained, []string{"z", "y"}) { + t.Errorf("unexplained = %q", unexplained) + } + if !slices.Equal(stale, []string{"a", "b"}) { + t.Errorf("stale = %q", stale) } } diff --git a/internal/refs/allow.go b/internal/refs/allow.go deleted file mode 100644 index 13861fd..0000000 --- a/internal/refs/allow.go +++ /dev/null @@ -1,46 +0,0 @@ -package refs - -import ( - "fmt" - - "github.com/spin-stack/go-tools/internal/allow" -) - -// Allow is the list of references made on purpose to something that is not here - naming a -// thing to say it is gone, or naming something in another repository. -type Allow struct { - keys map[string]bool -} - -// ReadAllow parses ` # why`, the why required. No path is no allowlist: -// every reference must resolve. -func ReadAllow(path string) (*Allow, error) { - keys, err := allow.Read(path) - if err != nil { - return nil, fmt.Errorf("refs: %w", err) - } - return &Allow{keys: keys}, nil -} - -// Len is how many exceptions the list holds. -func (a *Allow) Len() int { return len(a.keys) } - -// Split turns findings into the two halves the ratchet reports: what nothing explains, and -// what the list explains but no longer happens. -// -// The second half is the direction people forget. An entry that matches nothing means the -// sentence was rewritten or the thing came back, and a list that keeps it goes on excusing a -// reference nobody makes - which is how an allowlist stops being a ratchet and becomes a -// place to put things. -func (a *Allow) Split(found []Finding) (unexplained []Finding, stale []string) { - matched := map[string]bool{} - for _, f := range found { - key := f.Key() - if a.keys[key] { - matched[key] = true - continue - } - unexplained = append(unexplained, f) - } - return unexplained, allow.Stale(a.keys, matched) -} diff --git a/internal/refs/refs.go b/internal/refs/refs.go index 78c5a7e..37e0d51 100644 --- a/internal/refs/refs.go +++ b/internal/refs/refs.go @@ -35,7 +35,9 @@ type Finding struct { Ref string } -// Key is the allowlist's spelling of a finding. +// Key is the allowlist's spelling of a finding: ``. An allowlist names the +// references made on purpose to something that is not here - naming a thing to say it is gone, or +// naming something in another repository. func (f Finding) Key() string { return f.File + "\t" + f.Ref } // TaskLister answers what `task` names resolve in a directory. It returns no error and no diff --git a/internal/refs/refs_test.go b/internal/refs/refs_test.go index 2785651..8a119ed 100644 --- a/internal/refs/refs_test.go +++ b/internal/refs/refs_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "testing" + "github.com/spin-stack/go-tools/internal/allow" "github.com/spin-stack/go-tools/internal/refs" ) @@ -212,15 +213,12 @@ func TestTheAllowlistTurnsBothWays(t *testing.T) { t.Fatal(err) } - allow, err := refs.ReadAllow(allowPath) + allowed, err := allow.Read(allowPath) if err != nil { t.Fatalf("read allow: %v", err) } - if allow.Len() != 2 { - t.Fatalf("the list holds %d entries, wanted 2", allow.Len()) - } - unexplained, stale := allow.Split(check(t, root, onlyRoot("build"))) + unexplained, stale := allow.Split(allowed, check(t, root, onlyRoot("build")), refs.Finding.Key) wants(t, unexplained, "internal/audit") if len(stale) != 1 || stale[0] != "CLAUDE.md\tinternal/ancient" { t.Fatalf("stale entries are %q, wanted the one nothing matches", stale)