|
| 1 | +## Context |
| 2 | + |
| 3 | +`fname` is a small Go library and CLI for generating human-friendly random name phrases. The codebase is compact (~150 LOC across 3 Go files) but has accumulated several bugs and rough edges. The public library API is used externally (`go get github.com/splode/fname`), so breaking changes require care. |
| 4 | + |
| 5 | +Current pain points: |
| 6 | +- The seed `-1` sentinel makes a valid int64 input silently unusable |
| 7 | +- An index-based collision loop guards against a problem that doesn't exist |
| 8 | +- Size validation happens too late (at generate time, not construction time) |
| 9 | +- The word-list parser uses streaming I/O on in-memory strings |
| 10 | +- Verb tenses are inconsistent across the 448-word verb list |
| 11 | +- 72 words appear in both adjective and noun lists |
| 12 | + |
| 13 | +## Goals / Non-Goals |
| 14 | + |
| 15 | +**Goals:** |
| 16 | +- Fix the seed sentinel bug with a clean API that accepts all int64 values |
| 17 | +- Remove dead/incorrect logic (collision loop) |
| 18 | +- Validate generator options eagerly at construction time |
| 19 | +- Improve word list quality (verb tense consistency, cross-list overlap) |
| 20 | +- Minor performance improvements (split, casing switch) |
| 21 | +- Add `WithDictionary()` to fulfill the existing TODO |
| 22 | +- Add `--format` output flag to the CLI |
| 23 | + |
| 24 | +**Non-Goals:** |
| 25 | +- Rewriting the generator architecture |
| 26 | +- Changing the default name format or word order |
| 27 | +- Expanding the size range beyond 2–4 |
| 28 | +- Adding cryptographic randomness |
| 29 | + |
| 30 | +## Decisions |
| 31 | + |
| 32 | +### D1: Seed flag uses `*int64` instead of sentinel `-1` |
| 33 | + |
| 34 | +**Decision**: Change the `seed` variable in `main()` from `int64 = -1` to `*int64` (nil = unset). Pass `fname.WithSeed(*seed)` only when non-nil. |
| 35 | + |
| 36 | +**Alternatives considered**: |
| 37 | +- Separate `--no-seed` bool flag: adds a flag just to undo another flag, confusing |
| 38 | +- Use `0` as sentinel: same problem, `0` is a valid seed |
| 39 | +- `*int64` nil pointer: idiomatic Go for "optional value", clean, no reserved values |
| 40 | + |
| 41 | +**Impact**: CLI-only change. The library's `WithSeed(int64)` signature is unchanged. |
| 42 | + |
| 43 | +### D2: Remove collision-avoidance loop, no replacement |
| 44 | + |
| 45 | +**Decision**: Delete the `for adjectiveIndex == nounIndex` loop entirely. No replacement needed. |
| 46 | + |
| 47 | +**Rationale**: The loop compares an index into the adjective list (0..1745) against an index into the noun list (0..2663). Equal integers do not mean equal words — `adjective[5]="absolute"`, `noun[5]="absence"`. The loop solves a phantom problem and could theoretically spin indefinitely on equal-length lists. |
| 48 | + |
| 49 | +**Alternatives considered**: |
| 50 | +- Fix the loop to compare word strings: adds a real-collision check, but two-word names from a 4.6M combination space make same-word collisions negligible (~0.07% for largest overlap category) |
| 51 | +- Keep loop as-is: incorrect semantics, wasted cycles |
| 52 | + |
| 53 | +### D3: Eager size validation in `WithSize` |
| 54 | + |
| 55 | +**Decision**: Return an error from `WithSize()`, changing its signature to `func WithSize(size uint) (GeneratorOption, error)`. Callers get immediate feedback on invalid sizes. |
| 56 | + |
| 57 | +**Alternatives considered**: |
| 58 | +- Validate in `NewGenerator()` and return `(*Generator, error)`: larger API change, but cleaner; deferred for a future refactor |
| 59 | +- Keep deferred validation in `Generate()`: current state, poor library ergonomics |
| 60 | +- Panic in `WithSize()`: not idiomatic Go for user input validation |
| 61 | + |
| 62 | +**Note**: This is a **BREAKING** change to the `WithSize` function signature. |
| 63 | + |
| 64 | +### D4: Replace `bufio.Scanner` with `strings.Split` |
| 65 | + |
| 66 | +**Decision**: Replace the `split()` function body with `strings.Split(strings.TrimRight(s, "\n"), "\n")`. The embedded data is already in memory; a streaming scanner adds unnecessary overhead. |
| 67 | + |
| 68 | +**Rationale**: Simpler, faster, no reader allocation. The only edge case is a trailing newline on the embedded string, which `strings.TrimRight` handles. |
| 69 | + |
| 70 | +### D5: Replace `casingMap` with a `switch` in `applyCasing` |
| 71 | + |
| 72 | +**Decision**: Remove `casingMap` and replace the map lookup in `applyCasing` with a direct `switch` on `g.casing`. |
| 73 | + |
| 74 | +**Rationale**: Three cases don't benefit from a map. A switch is zero-allocation, branch-predictor friendly, and more readable. |
| 75 | + |
| 76 | +### D6: Document (not fix) goroutine safety |
| 77 | + |
| 78 | +**Decision**: Add a doc comment to `Generator` stating it is not safe for concurrent use. Creating a new `Generator` per goroutine is the idiomatic solution. |
| 79 | + |
| 80 | +**Alternatives considered**: |
| 81 | +- Add a `sync.Mutex` around rand calls: adds lock contention overhead for the common single-goroutine case |
| 82 | +- Switch to `math/rand/v2` global rand: changes minimum Go version requirement and behavior |
| 83 | + |
| 84 | +### D7: `WithDictionary()` takes a `*Dictionary` |
| 85 | + |
| 86 | +**Decision**: Add `WithDictionary(d *Dictionary)` as a new `GeneratorOption`. `NewDictionary()` remains the default; callers who want custom words construct their own `Dictionary` and pass it in. |
| 87 | + |
| 88 | +**Rationale**: Minimal API surface, composable with existing options, fulfills the existing TODO with no breakage. |
| 89 | + |
| 90 | +### D8: `CasingFromString` → `ParseCasing` |
| 91 | + |
| 92 | +**Decision**: Rename `CasingFromString` to `ParseCasing`. Keep `CasingFromString` as a deprecated alias for one release cycle. |
| 93 | + |
| 94 | +**Rationale**: `ParseX` is the idiomatic Go convention for string-to-value parsing (cf. `strconv.ParseInt`, `time.Parse`). |
| 95 | + |
| 96 | +### D9: `--format` flag with `plain` (default) and `json` |
| 97 | + |
| 98 | +**Decision**: Add `--format` / `-f` flag accepting `plain` (default, current behavior) or `json`. JSON output is an array of name strings: `["name1","name2"]`. |
| 99 | + |
| 100 | +**Rationale**: Enables scripting without `xargs` / `sed` gymnastics. An array is more useful than newline-delimited JSON objects for batch generation. |
| 101 | + |
| 102 | +## Risks / Trade-offs |
| 103 | + |
| 104 | +- **`WithSize` signature change is breaking** → Mitigated by it being a small, focused library; version bump to communicate the change |
| 105 | +- **Verb list edits are manual and subjective** → Mitigated by focusing on clearly wrong tenses (past participles like "abandoned" in a verb slot that reads as present action); a full linguistic audit is out of scope |
| 106 | +- **Adj/noun overlap cleanup could remove intentionally dual-use words** → Mitigated by only removing from the list where the word is clearly stronger in one category (e.g., "blue" as adjective, not noun) |
| 107 | + |
| 108 | +## Open Questions |
| 109 | + |
| 110 | +- Should `NewGenerator` be changed to return `(*Generator, error)` now, or deferred to a future version? (Current proposal keeps it non-error-returning for minimal breakage) |
| 111 | +- Should JSON output for `--format json` include metadata (seed used, size, count)? Or just the name array? |
0 commit comments