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)