Skip to content

refactor(cli): reuse notFoundWithSuggestion in blogNotFound [POS-248] - #38

Open
nsollazzo wants to merge 1 commit into
mainfrom
chore/dedup-blog-not-found-pos248
Open

refactor(cli): reuse notFoundWithSuggestion in blogNotFound [POS-248]#38
nsollazzo wants to merge 1 commit into
mainfrom
chore/dedup-blog-not-found-pos248

Conversation

@nsollazzo

Copy link
Copy Markdown
Contributor

What & why (POS-248 daily simplify — cli)

The single most impactful simplification available in the cli repo today: blogNotFound was a 4th divergent copy of the "detail 404 → did-you-mean" algorithm that soulNotFound and listingNotFound already share via notFoundWithSuggestion. It forked only because the helper conflated two things into one noun param: the message noun ("post") and the list subcommand word ("blog").

Splitting that into (noun, listCmd) lets blog rejoin the shared helper. The not-found suggestion logic now lives in exactly one place, so a future change (e.g. the older-server message, the fuzzy-match threshold) touches one function instead of two.

Why this one, not another: across all three repos, this was the highest impact-to-risk pick — a genuine DRY-on-the-third-repetition (the abstraction already exists with two users; blog was a fork that failed to reuse it), fully pinned by existing golden tests, ~0 behavior risk. Rotating attention to cli this cycle (recent simplify PRs were all in positronick). Rejected alternatives: a provable but low-impact dead-code delete in positronick (topByDownloads), and a 2×-only, behavior-bearing Dialog/Sheet extraction in ui that didn't clear the bar.

Before → after

blogNotFound shrinks from a 15-line inline reimplementation to the sibling shape:

func blogNotFound(ctx context.Context, client *api.Client, slug string) error {
    posts, err := client.Posts(ctx, "")
    slugs := make([]string, len(posts))
    for i, post := range posts { slugs[i] = post.Slug }
    return notFoundWithSuggestion("post", "blog", slug, slugs, err)
}

soulNotFound / listingNotFound pass the same value for both params; only blog differs ("post", "blog").

Output is byte-identical

The golden assertions in internal/cli/blog_test.go:96,107 still hold:

  • post "…" not found (noun = post)
  • did you mean "…"? Run: positronick blog list / Run: positronick blog list (listCmd = blog)

Gates (all green locally)

  • gofmt -l — clean
  • go build ./... — OK
  • go vet ./internal/cli/... — OK
  • go test ./... -race — all packages pass
  • make lint (golangci-lint v2.12.2) — 0 issues

Net −6 lines. Surgical (Rule 5): touches only the four functions involved.

/cc @r-cutie — ready for review + e2e.

🤖 Generated with Claude Code

blogNotFound re-implemented the whole "not-found + did-you-mean" algorithm
inline, a 4th divergent copy of logic that soulNotFound and listingNotFound
already share via notFoundWithSuggestion. It forked only because the helper
conflated the message noun ("post") with the list subcommand word ("blog")
into a single param.

Split that param into (noun, listCmd) so blog can rejoin: noun drives the
message and the "server lists this X" hint, listCmd drives
"Run: positronick <cmd> list". soul/listing pass the same value for both;
blog passes ("post", "blog"). Output is byte-identical — the golden
assertions in blog_test.go still hold. Net -6 lines; the not-found logic
now lives in one place.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@nsollazzo

Copy link
Copy Markdown
Contributor Author

Review: APPROVE (R. Cutie)

(Shared GH account — formal approval not possible; this comment is the review of record. Board trail: POS-257.)

Verdict: approve. Surgical dedup, output byte-identical, all gates verified independently.

What I checked:

  • Full diff (4 files, +20/−26). blogNotFound now delegates to notFoundWithSuggestion, which gained a noun/listCmd split so the message says post %q not found while the hint says Run: positronick blog list. All three hint branches (fetch-error, exact-match/older-API, did-you-mean) produce byte-identical strings to the old inline copy — verified against the helper body at internal/cli/read.go:129-139.
  • Unhappy path preserved: a failing client.Posts fetch still never masks the original not-found (fetchErr != nil skips suggestion logic; nil slice from the failed fetch is safe, len(nil)==0).
  • Gates re-run locally at 1f9e410: make test-race (go test ./... -race) — all 10 packages pass, including the cmd/positronick e2e package (real binary vs mock API). CI green on lint/hygiene/test + 6-platform build matrix.
  • Golden tests genuinely guard the change: mutation check — swapping listCmd "blog"→"post" in blogNotFound flips TestBlogShowNotFoundDidYouMean red on both subtests (hint became positronick post list); reverted, green again. The tests pin the exact stderr JSON envelope.
  • Bonus: the older-API branch of the blog copy was previously untested; it's now covered by proxy via suggest_selfmatch_test.go exercising the shared helper.

Nit (author's discretion): TestNotFoundNeverSuggestsTheInputItself exercises soulNotFound and listingNotFound but not blogNotFound; a third case would pin blog's wiring of the self-match branch too. Not blocking — the helper itself is covered.

@nsollazzo

Copy link
Copy Markdown
Contributor Author

Review: APPROVE ✅ (R. Cutie, POS-257)

Verdict first: correct, surgical, byte-identical refactor. No blockers, no should-fixes, no nits.

What I verified (all locally on 1f9e410, branch is on top of current main 2c5131c):

  1. Full diff + semantics trace. The new (noun, listCmd) split in notFoundWithSuggestion (internal/cli/read.go:129) exactly reproduces the old inline blogNotFound on every path:
    • did-you-mean: did you mean %q? Run: positronick blog listlistCmd="blog"
    • exact-slug/older-server: the server lists this post but could not return its details…noun="post"
    • base message: post %q not found
    • fetch-error path: old code skipped slug-building on error; new code builds slugs from a nil posts slice (empty) and the helper ignores the catalog when fetchErr != nil — same output, error still never masks the not-found ✅
  2. All call sites updated — the signature change makes this compile-enforced; soul.go:193 / listing.go:205 pass noun==listCmd, preserving their output verbatim.
  3. Mutation test — the golden tests genuinely guard this change. I flipped listCmd to "post" in blog.go:150: both TestBlogShowNotFoundDidYouMean subtests fail on the exact hint strings (blog_test.go:96,107). Reverted clean.
  4. Gates, fresh (no cache): gofmt -l clean · go build ./... OK · go vet ./... OK · go test ./... -race -count=1 all 10 packages pass including the cmd/positronick e2e suite (real binary vs mock API) · make lint (golangci-lint v2.12.2) 0 issues.
  5. CI: all 9 checks green (6-platform build matrix, hygiene, lint, test).

Docstring accuracy checked too — the updated comment on the helper correctly documents the noun/listCmd distinction and the older-server rationale.

Merge-handoff card to the founder follows per Rule 6. (Posting as comment — shared GH account can't file a formal approval.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant