Decompose compileTemplate, checkBoundLevelsHeld and loadDir; gate cyclomatic complexity at 14

Dockerfile

@@ -13,6 +13,7 @@ RUN go mod download
 
 COPY . .
 RUN go vet ./... && \
+	go run github.com/fzipp/gocyclo/cmd/gocyclo@v0.6.0 -over 14 -ignore _test . && \
 	{ unformatted="$(gofmt -l .)"; test -z "$unformatted" || \
 	  { echo "unformatted files:"; echo "$unformatted"; exit 1; }; } && \
 	go test -race ./...

README.md

@@ -379,6 +379,7 @@ docker compose run --rm cover   # tests with coverage
 docker compose run --rm bench   # benchmarks
 docker compose run --rm build   # compile the library
 docker compose run --rm vet     # go vet
+docker compose run --rm cyclo   # cyclomatic complexity over 14 (test files excluded)
 docker compose run --rm dev     # interactive shell
 ```
 
@@ -391,7 +392,7 @@ docker compose run --rm --user "$(id -u):$(id -g)" tidy  # go mod tidy
 
 Every pull request runs `docker build .` against both the latest and the lowest
 supported Go, and must pass before it can be merged. That build is the whole
-gate — vet, format check and tests — so run it locally before pushing:
+gate — vet, complexity, format check and tests — so run it locally before pushing:
 
 ```sh
 docker build .                                  # latest

calc.go

@@ -242,6 +242,7 @@ func (p *calcParser) binary(next func() (calcNode, error), ops ...byte) (calcNod
 	}
 }
 
+// factor is a table-shaped scanner, one case per token kind, kept whole on purpose.
 func (p *calcParser) factor() (calcNode, error) {
 	p.space()
 	if p.pos >= len(p.rs) {

compose.yaml

@@ -42,6 +42,10 @@ services:
     <<: *go
     command: go vet ./...
 
+  cyclo:
+    <<: *go
+    command: go run github.com/fzipp/gocyclo/cmd/gocyclo@v0.6.0 -over 14 -ignore _test .
+
   build:
     <<: *go
     command: go build ./...

data.go

@@ -87,42 +87,57 @@ func loadDir(src dataSource, dir string) (*group, error) {
 			continue
 		}
 		full := path.Join(dir, e.Name())
+		load := loadFile
 		if e.IsDir() {
-			child, err := loadDir(src, full)
-			if err != nil {
+			load = loadFolder
+		}
+		if err := load(src, g, full, e.Name()); err != nil {
 			return nil, err
 		}
+	}
+	return g, nil
+}
+
+// loadFolder adds a subdirectory as a nested group, unless nothing under it is data.
+func loadFolder(src dataSource, g *group, full, name string) error {
+	child, err := loadDir(src, full)
+	if err != nil {
+		return err
+	}
 	if len(child.children) == 0 {
-				continue
+		return nil
 	}
-			if err := checkName(e.Name()); err != nil {
-				return nil, fmt.Errorf("%s: folder %w", src.name(full), err)
-			}
-			g.children[e.Name()] = child
-			continue
-		}
-		if !strings.HasSuffix(e.Name(), ".json") {
-			continue
-		}
-		name := strings.TrimSuffix(e.Name(), ".json")
 	if err := checkName(name); err != nil {
-			return nil, fmt.Errorf("%s: category %w", src.name(full), err)
+		return fmt.Errorf("%s: folder %w", src.name(full), err)
+	}
+	g.children[name] = child
+	return nil
+}
+
+// loadFile compiles a *.json file into a category named after it; any other file
+// is skipped.
+func loadFile(src dataSource, g *group, full, file string) error {
+	if !strings.HasSuffix(file, ".json") {
+		return nil
+	}
+	name := strings.TrimSuffix(file, ".json")
+	if err := checkName(name); err != nil {
+		return fmt.Errorf("%s: category %w", src.name(full), err)
 	}
 	b, err := fs.ReadFile(src.fsys, full)
 	if err != nil {
-			return nil, fmt.Errorf("%s: %w", src.name(full), err)
+		return fmt.Errorf("%s: %w", src.name(full), err)
 	}
 	var raw any
 	if err := json.Unmarshal(b, &raw); err != nil {
-			return nil, fmt.Errorf("%s: %w", src.name(full), err)
+		return fmt.Errorf("%s: %w", src.name(full), err)
 	}
 	n, err := compile(raw)
 	if err != nil {
-			return nil, fmt.Errorf("%s: %w", src.name(full), err)
+		return fmt.Errorf("%s: %w", src.name(full), err)
 	}
 	g.children[name] = n
-	}
-	return g, nil
+	return nil
 }
 
 // mergeChildren overlays src onto dst. Two groups under the same key merge

hold.go

@@ -19,13 +19,24 @@ func checkBoundLevelsHeld(root map[string]node) error {
 		if !ok || len(t.held) == 0 {
 			return nil
 		}
+		readers := boundReaders(t.format, t.bound, t.refs)
+		for _, head := range heldHeads(t) {
+			if err := checkHeadHeld(t, head, readers); err != nil {
+				return fmt.Errorf("%s: %w", path, err)
+			}
+		}
+		return nil
+	})
+}
+
+// heldHeads lists a template's held names, operand heads first, then paths, each
+// in name order: a level read both ways is reported by the operand's fence, and
+// which overlap is reported does not vary.
+func heldHeads(t *template) []string {
 	heads := make([]string, 0, len(t.held))
 	for head := range t.held {
 		heads = append(heads, head)
 	}
-		// Operand heads first, then paths, each in name order: a level read both
-		// ways is reported by the operand's fence, and which overlap is reported
-		// does not vary.
 	sort.Slice(heads, func(i, j int) bool {
 		_, pi := t.bound[heads[i]]
 		_, pj := t.bound[heads[j]]
@@ -34,42 +45,46 @@ func checkBoundLevelsHeld(root map[string]node) error {
 		}
 		return heads[i] < heads[j]
 	})
-		readers := boundReaders(t.format, t.bound, t.refs)
-		for _, head := range heads {
-			// What one draw answers for depends on how the draw is read: a path pins
-			// the levels it passes through and the leaf it lands on, an operand
-			// exactly the value its render produces.
+	return heads
+}
+
+// pinned collects what the hold of head answers for: a path pins the levels it
+// passes through and the leaf it lands on, an operand exactly the value its render
+// produces.
+func pinned(t *template, head string, readers []reader) map[node]bool {
 	held := map[node]bool{}
-			reader, isPath := t.bound[head]
-			if isPath {
+	if _, isPath := t.bound[head]; !isPath {
+		operandDraw(t.fields[head], held)
+		return held
+	}
 	for _, r := range readers {
 		if a := splitArm(r.name, t.refs); a.key == head {
 			coverPath(t.fields[head], a.tail, held)
 		}
 	}
-			} else {
-				operandDraw(t.fields[head], held)
-			}
+	return held
+}
+
+// checkHeadHeld rejects every route to what head's hold pins except the readers
+// holding it. One seen set across the edges: a node that cannot reach the level
+// cannot reach it by another route either, so it is walked once.
+func checkHeadHeld(t *template, head string, readers []reader) error {
+	held := pinned(t, head, readers)
 	if len(held) == 0 {
-				continue // an early out: a fixed head holds nothing to reach
+		return nil // a fixed head holds nothing to reach
 	}
-			// One seen set across the edges: a node that cannot reach the level
-			// cannot reach it by another route either, so it is walked once here.
+	reader, isPath := t.bound[head]
 	seen := map[node]bool{}
 	for _, e := range renderEdges(t) {
-				if splitArm(e.label, t.refs).key == head {
-					continue // a token or operand reading this draw, the routes allowed
+		if splitArm(e.label, t.refs).key == head || !renders(e.to, held, seen) {
+			continue
 		}
-				if renders(e.to, held, seen) {
 		if isPath {
-						return fmt.Errorf("%s: %s renders %q, which {%s} reads a path into; name the fields you want instead", path, e.reached(), head, reader)
-					}
-					return fmt.Errorf("%s: %s renders %q, which a {%s()} also reads; reach it one way so it is drawn once", path, e.reached(), head, operandReader(t, head))
-				}
+			return fmt.Errorf("%s renders %q, which {%s} reads a path into; name the fields you want instead", e.reached(), head, reader)
 		}
+		return fmt.Errorf("%s renders %q, which a {%s()} also reads; reach it one way so it is drawn once", e.reached(), head, operandReader(t, head))
 	}
 	return nil
-	})
 }
 
 // operandReader names the builtin whose operand holds head.

node.go

@@ -181,29 +181,68 @@ func checkNoRepeatedItem(items []any) error {
 }
 
 func compileTemplate(m map[string]any) (node, error) {
-	format, ok := m["format"].(string)
-	if !ok {
-		return nil, fmt.Errorf("template object missing string \"format\"")
-	}
-	repeat, err := repeatOf(m)
+	o, err := readOptions(m)
 	if err != nil {
 		return nil, err
 	}
-	sep := ""
+	fields, err := compileFields(m)
+	if err != nil {
+		return nil, err
+	}
+	if len(fields) == 0 && o.repeat == 1 && !o.weighted {
+		return nil, fmt.Errorf("an object holding only a format is a string; write %q", o.format)
+	}
+	if err := checkTokens(o.format, fields); err != nil {
+		return nil, err
+	}
+	t := &template{format: o.format, fields: fields, repeat: o.repeat, separator: o.separator}
+	if err := t.compileFormat(); err != nil {
+		return nil, err
+	}
+	return t, nil
+}
+
+// templateOptions is what a template object's option keys say.
+type templateOptions struct {
+	format    string
+	repeat    int
+	separator string
+	weighted  bool
+}
+
+func readOptions(m map[string]any) (templateOptions, error) {
+	var o templateOptions
+	format, ok := m["format"].(string)
+	if !ok {
+		return o, fmt.Errorf("template object missing string \"format\"")
+	}
+	o.format = format
+	repeat, err := repeatOf(m)
+	if err != nil {
+		return o, err
+	}
+	o.repeat = repeat
 	if sv, ok := m["separator"]; ok {
-		if sep, ok = sv.(string); !ok {
-			return nil, fmt.Errorf("separator must be a string, got %T", sv)
+		if o.separator, ok = sv.(string); !ok {
+			return o, fmt.Errorf("separator must be a string, got %T", sv)
 		}
 		if repeat == 1 {
-			return nil, fmt.Errorf("separator joins repeated renders, so it has no effect without a repeat above 1")
+			return o, fmt.Errorf("separator joins repeated renders, so it has no effect without a repeat above 1")
 		}
 	}
-	t := &template{format: format, fields: make(map[string]node, len(m)), repeat: repeat, separator: sep}
+	_, o.weighted = m["weight"]
+	return o, nil
+}
+
+// compileFields compiles every non-option key of a template object, in name order
+// so which of several bad fields is reported does not vary.
+func compileFields(m map[string]any) (map[string]node, error) {
+	fields := make(map[string]node, len(m))
 	keys := make([]string, 0, len(m))
 	for k := range m {
 		keys = append(keys, k)
 	}
-	sort.Strings(keys) // so which of several bad fields is reported does not vary
+	sort.Strings(keys)
 	for _, k := range keys {
 		if isOption(k) {
 			continue
@@ -215,18 +254,9 @@ func compileTemplate(m map[string]any) (node, error) {
 		if err != nil {
 			return nil, fmt.Errorf("field %q: %w", k, err)
 		}
-		t.fields[k] = n
+		fields[k] = n
 	}
-	if _, weighted := m["weight"]; len(t.fields) == 0 && repeat == 1 && !weighted {
-		return nil, fmt.Errorf("an object holding only a format is a string; write %q", format)
-	}
-	if err := checkTokens(format, t.fields); err != nil {
-		return nil, err
-	}
-	if err := t.compileFormat(); err != nil {
-		return nil, err
-	}
-	return t, nil
+	return fields, nil
 }
 
 // repeatOf reads a template's "repeat" (default 1): how many times its format

template.go

@@ -15,7 +15,8 @@ type ftoken struct {
 
 // eachToken scans a format string once and calls fn for each unit, the single
 // source of truth for how braces are read: "{{" and "}}" are literal braces, a "{"
-// opens a token that must reach its "}", and a lone "}" is an error.
+// opens a token that must reach its "}", and a lone "}" is an error. A table-shaped
+// scanner, one case per rune kind, kept whole on purpose.
 func eachToken(format string, fn func(ftoken) error) error {
 	rs := []rune(format)
 	for i := 0; i < len(rs); i++ {