refactor: consolidate paging/print helpers + split buildTagPayload + close test gaps#23
Merged
Merged
Conversation
…close test gaps Follow-up to fix/api-error-envelope-and-safety. Now that every print* helper goes through the same respond() path, the per-resource families (investment_helpers, financial_cmds, family_exports, users, imports paging) are byte-identical except for the prefix in the function name. Collapse them. ## Helper consolidation - Delete printInvestmentGet/Post/Patch/Delete/DryRun (investment_helpers.go:55-112). All callers in investments_cmds.go + trades_cmd.go + holdings_cmd.go migrate to printGet / dispatchWrite. The only thing kept from that file is addRepeatedQuery (renamed from addRepeatedInvestmentQuery — the helper is generic). - Delete printFinancialGet/Post/Patch/DryRun (financial_cmds.go:260-310). Valuations create/update migrate to dispatchWrite; everything else goes through printGet. - Delete printFamilyExportGet/DryRun (family_exports_cmd.go:90-111). list/show migrate to printGet; create migrates to dispatchWrite. Preserved the no-body dry-run shape upstream relies on. - Delete printUsersGet/Delete/DryRun (users_cmd.go:57-86). reset / reset status / delete-me migrate to dispatchWrite + printGet. - Delete addImportPagingFlags/Query, importPathWithQuery (imports_cmd.go:266-291) — same shape as the reference_cmds.go trio, no behavioural delta. - Delete addInvestmentPagingFlags/Query, investmentPathWithQuery (investment_helpers.go:12-39). - Delete addFinancialPagingFlags/Query, financialPathWithQuery (financial_cmds.go:231-258). ## Pattern fix — buildTagPayload flag argument Split buildTagPayload(o, requireName bool) at reference_cmds.go:233 into separate buildTagCreatePayload (rejects empty Name) and buildTagUpdatePayload (rejects "no fields provided to update"). Matches the chats / valuations / trades / categories pattern of distinct create/update builders. Two 10-line funcs instead of one 12-line func with a magic boolean. ## Test helpers hoisted - New testhelpers_test.go with findSub + captureStdout. Both were hand-rolled in transfers_cmds_test.go and dispatch_write_test.go respectively but used across the whole package. Centralizing them avoids the "where does findSub live?" confusion called out in the refactor review. ## Test gaps closed - New orphan_commands_test.go with a registration smoke table for whoami, sync, refresh, login, status, export[transactions], transactions delete, insights[subscriptions/fees/leaks], plan[budget/runway/forecast], propose[rules]. Previously each of those commands had zero test coverage — deleting the AddCommand line in root.go would have silently still compiled. Now it fails fast. - reference_cmds_test.go: replaced the single TestBuildTagPayload with four focused tests covering create-required-name, create-color-optional, update-needs-at-least-one-field, update-partial-ok. Also tightened TestReferenceCommandsRegistered to compare resolved cmd.Name() to the expected leaf (cobra's loose Find pattern that bit us before). ## Coverage cmd/sure-cli/root: 47.6% → 52.9% (+5.3pp). go test ./... -race -cover green across the suite. ## Diff scope 14 files changed, +233/-391 = net **−158 LOC**. Pure mechanical collapse — every migrated site already had behavioural test coverage via the previous PR's respond() tests or the existing command-shape tests, and no functional change is intended. Refs the post-merge code review.
superagent-security
Bot
added
the
contributor:verified
Contributor
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 52 minutes and 1 second. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #22. Now that every `print*` helper routes through the same `respond()` path, the per-resource families are byte-identical except for the prefix in the function name. This PR collapses them, splits the `buildTagPayload` flag-argument smell, and closes the orphan-command test gaps flagged in the post-merge review.
Helper consolidation (net −158 LOC)
The only thing kept from `investment_helpers.go` is `addRepeatedQuery` (renamed from `addRepeatedInvestmentQuery` — it's generic, used by trades + holdings).
Pattern fix — `buildTagPayload`
Split `buildTagPayload(o, requireName bool)` (`reference_cmds.go:233`, flag-argument smell) into separate `buildTagCreatePayload` and `buildTagUpdatePayload`. Matches the chats / valuations / trades / categories pattern of distinct create/update builders.
Test infrastructure
Coverage
`cmd/sure-cli/root`: 47.6% → 52.9% (+5.3pp). `go test ./... -race -cover` green across the suite.
Scope
14 files changed, +233/-391. Pure mechanical collapse — every migrated site already had behavioural coverage via #22's `respond()` tests or existing command-shape tests. No functional change intended; the dry-run envelope shape for `family-exports create` is explicitly preserved (no `body` key on the dry-run side; `{}` only on `--apply`).
Refs the post-merge code review.