Skip to content

refactor: dedup admin field-flag application into applyStringFieldFlags (POS-187) - #36

Merged
nsollazzo merged 1 commit into
mainfrom
giskard/pos-187-dedup-field-flags
Jul 13, 2026
Merged

refactor: dedup admin field-flag application into applyStringFieldFlags (POS-187)#36
nsollazzo merged 1 commit into
mainfrom
giskard/pos-187-dedup-field-flags

Conversation

@nsollazzo

Copy link
Copy Markdown
Contributor

What & why (POS-187 daily code-health)

The four hidden admin commands — soul, post, feed, profile — each carried a near-identical loop that walks a [][2]string flag→API-key mapping and copies string flags into the JSON request body. feed's copy already generalized the two semantics via a forCreate bool; the other three just hadn't been pointed at it. This is the single clearest copy-paste cluster in the repo (the rest of internal/cli / internal/api / output is already well-factored), so it's today's highest impact-to-risk win.

Change

Extract the loop once as:

func applyStringFieldFlags(cmd *cobra.Command, mapping [][2]string, fields map[string]any, omitEmpty bool) error
  • omitEmpty=true (create): send only non-empty values → empty optionals fall through to server defaults.
  • omitEmpty=false (update): send every explicitly-set flag, incl. empty → clears the field.

All four call sites now delegate:

  • applySoulFieldFlags / applyPostFieldFlags…, false (keep their --framework / --tag array handlers untouched)
  • applyFeedStringFlags → thin wrapper passing forCreate through (call sites unchanged)
  • profile create inline loop → …, true

Before / after

  • Before: 4 copies of the ~10-line loop across admin.go, post.go, feed.go, profile.go.
  • After: one helper; call sites are 1–3 lines each. Net −11 lines, four copies → one.

Behavior

No behavior change. The only mechanical difference is that the shared helper calls GetString on every mapped flag rather than only changed ones — harmless, since every mapped flag is registered and the Changed/omitEmpty guard still decides inclusion. Wire-body ordering is preserved (ordered [][2]string).

Gates

  • go build ./...
  • go vet ./...
  • make fmt-check ✓ (clean)
  • make lint ✓ (golangci-lint v2.12.2, 0 issues)
  • make test-race ✓ (full suite incl. cmd/positronick e2e + golden wire-body assertions that pin --frameworkframeworks, --tagtags, --author-handle, create paths)

Never merged — handing to R. Cutie for review + e2e.

Co-Authored-By: Paperclip noreply@paperclip.ing

🤖 Generated with Claude Code

…gs (POS-187)

The soul/post/feed/profile admin commands each carried a near-identical
loop that walked a [][2]string flag→API-key mapping and copied string
flags into the request body. feed's version already generalized the two
semantics (create = omit empty, update = send changed incl. empty) via a
forCreate bool; the other three just hadn't been pointed at it.

Extract that loop once as applyStringFieldFlags(cmd, mapping, fields,
omitEmpty) and have all four call sites delegate to it. No behavior
change: the array (--framework/--tag) and bool handlers are untouched,
and the existing e2e/golden wire-body assertions still pass.

Net -11 lines, four copies of the loop collapsed to one.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@nsollazzo

Copy link
Copy Markdown
Contributor Author

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

Verdict: approve — faithful dedup, no behavior change, independently verified byte-identical wire bodies vs main.

Diff review

  • applyStringFieldFlags reproduces both prior semantics exactly: omitEmpty=true (feed/profile create) = old v != "" gate; omitEmpty=false (soul/post create+update, feed update) = old Changed() gate.
  • Checked every mapping/registrar pairing: soul 7/7, post 11/11, feed 6/6, profile 7/7 flags registered on every command that applies them — so the helper's now-unconditional GetString can't error at runtime. Behavior note (not a defect): a future mapping entry naming an unregistered flag now fails loud instead of being silently skipped when unchanged — an improvement.
  • Edge case verified: profile --kind has a non-empty default (person) and is still always sent on create, matching the old inline loop.
  • Array (--framework/--tag) and bool handlers untouched, confirmed in diff.

Independent e2e verification (mock API)

Built binaries from main (9eaafc4) and this branch, drove both through an identical 15-command sequence (soul/post/feed create + update, profile create) against a body-capturing wrapper around internal/mockapi, byte-diffed all 24 captured requests:

  • All wire bodies byte-identical, all exit codes identical.
  • Update clears confirmed: soul update --tagline "" --license ""{"license":"","tagline":""}; feed update --author ""{"authorHandle":""}.
  • Create omits confirmed: feed create --author "" omits authorHandle; profile create --bio "" omits bio while still sending kind: person default.
  • Deterministic key order preserved (alphabetical, per Go JSON map marshaling).

Gates (rerun locally on this branch)

go build ✅ · go vet ✅ · make fmt-check ✅ · make lint (0 issues) ✅ · make test-race full suite incl. cmd/positronick e2e + golden tests ✅

CI hygiene failure — pre-existing, NOT this PR

The red hygiene check is govulncheck flagging GO-2026-5856 (crypto/tls, stdlib, fixed in go1.26.5) via internal/auth/internal/selfupdate — files this PR doesn't touch. main's last run (Jul 8) passed because the advisory landed after; a re-run of main would fail identically. Fix is a toolchain bump to go1.26.5, tracked separately — do not treat it as a defect of this diff.

Merge remains a human action per policy.

@nsollazzo
nsollazzo merged commit 7826611 into main Jul 13, 2026
8 of 9 checks passed
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