CLI-1086 Propagate Result through the list projects command handler - #807
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
✅ Deploy Preview for sonarqube-cli canceled.
|
Convert listProjects() to return ResultAsync<void, InvalidOptionError | HttpClientError> instead of throwing internally, chaining validation and the domain-client call with errAsync()/map() end to end. authenticatedAction() now accepts either a plain Promise<void> handler (unchanged for every other command) or a Result-returning one, collapsing the latter via match() at the same single point runCommand()'s try/catch already handles thrown errors - orThrow() can't be called directly on the resolved Ok<T, Error> | Err<T, Error> union since TypeScript can't unify the two branches' polymorphic this. Scoped to this one command on purpose, to see what the end-to-end shape looks like before converting the other ~40 handlers CLI-1086 covers.
Registers a bun test preload (tests/_common/result-matchers.ts, wired in
bunfig.toml alongside the two existing preloads) so any spec can assert
directly on an already-resolved Result without unwrapping it by hand first -
.orThrow() throws inside the test, so a failure reports as an uncaught
exception instead of a clear expected/received diff.
Decoupled from migrating the ~85 other .orThrow()-based test call sites
CLI-1086 also covers: the ticket treats that migration as gradual ("where
doing so reads more clearly"), so this only adds the matchers and dogfoods
them in the one test file this PR already touches (projects-command.test.ts),
plus a small dedicated test for the matchers' own pass/fail logic.
- Reuse isResult() from src/core/result.ts instead of re-implementing an equivalent (and already slightly diverging) duck-type check locally. - Branch each matcher's equality/predicate message on this.isNot, so a negated assertion that actually fails (e.g. .not.toBeOkWith(x) when the value does equal x) reports why instead of the self-contradictory "expected Ok(x) but got Ok(x)". Add tests that exercise that path plus the non-Result guard throw, since the existing suite only ever hit message() through already-pass:false branches.
Enables neverthrow/must-use-result (catches a Result built but never mapped/chained/matched/collapsed) on src/core/result.ts, src/core/commands/sonar-command.ts, and src/commands/list/projects.ts only - the files this spike PR actually converted. The commonly-recommended eslint-plugin-neverthrow hasn't published since 2021 and hard-crashes under this repo's ESLint 10 + typescript-eslint 8 (it reads context.parserServices, an API removed since); using the @ninoseki/eslint-plugin-neverthrow fork instead, which keeps the same rule working for current ESLint. Not enabled repo-wide: doing so surfaces ~17 pre-existing unconsumed Results in files outside this PR's scope (CLI-844's own conversion of the domain-client layer). That rollout, and fixing those sites, is its own follow-up once more command handlers migrate under CLI-1086.
c2ecc5f to
968cfac
Compare
The rule's handled-method list (match/unwrapOr/_unsafeUnwrap) is hardcoded with no options (schema: []), so it doesn't recognize orThrow() - this repo's actual collapse method - as consuming a Result. The previous comment called the pre-existing sites a repo-wide rollout would flag "unconsumed Results", implying real defects; most are correctly-collapsed .orThrow() chains the rule can't see. Corrected the rationale and shrunk the comment to the durable why.
listProjects() returned ResultAsync<void, InvalidOptionError | HttpClientError>, mixing a CliError (exitCode/remediationHint) with the raw domain-client error union that carries neither. mapErr() now wraps the domain-client failure into a CommandFailedError, so the whole rail speaks one vocabulary (ResultAsync<void, CliError>) - same message, exit code, and remediation hint as before, since CommandFailedError's constructor already derives the hint from the wrapped cause exactly as runCommand()'s catch did directly. Also made toBeErrWith()'s no-argument branch branch its message on this.isNot like every other branch in the file, instead of hardcoding one string.
…t-list-projects' into task/cs/CLI-1086-propagate-result-list-projects
…opagate-result-list-projects
…ess wrap The previous commit wrapped the domain failure in a CommandFailedError to make the signature read CliError. That wrap added nothing: same message, same exit code 1, and the same remediation hint, since CliError re-derives it from cause. Its only effect was cosmetic on the type, and replicated across the other ~40 handlers it would be a line of ceremony each. The repo's actual convention (import/index.ts) wraps in CommandFailedError when it has something to add - context in the message, an explicit hint. Declaring Error instead matches exactly what authenticatedAction() accepts, keeps runCommand() as the single place an error becomes an exit code, and leaves a real wrap available where a command genuinely has context to add. Exit codes are unchanged and covered by the integration spec (2 for InvalidOptionError, 1 for an API failure). Also adds the missing spec for the negated no-argument toBeErrWith() message, and notes in eslint.config.js that the neverthrow rule's file list is hand-maintained, so the next converted handler gets added rather than silently escaping the rule.
Three related simplifications, all stemming from the middle one.
The collapse read match(() => undefined, (error) => { throw error }) - a roundabout way to say "rethrow if it failed". Narrowing with isErr() and throwing directly says it in one line.
That in turn removes the reason for most of the JSDoc: the paragraph existed to justify why match() was used over orThrow(), a choice no longer being made. What is left is the durable why - the error is rethrown rather than handled so runCommand() stays the single collapse point.
The handler's return type moves to a named AuthenticatedCommandResult alias beside the existing CommandResult one, so the signature fits on a line and mirrors anonymousAction directly above it instead of inlining a union.
Also refreshes isResult()'s own doc, which described the caller's match() choice and would have gone stale again.
The added JSDoc argued the error-channel choice against the alternatives it rejected, which is review material, not something a future reader of this handler needs - and the rationale already lives in the pull request. The signature says ResultAsync<void, Error>; that is the whole contract. Also corrects the file's header comment, which said "Issues command - search for SonarQube issues" on the projects handler.
No behaviour change; comments, naming and shape only. result-matchers.ts: the two matchers each carried an identical five-line guard whose user-facing message had to stay in sync by hand, now one assertResult() helper. Header comment trimmed to the durable why and stripped of the ticket reference. The eslint-disable justification now cites TS2428, which is checkable, instead of asserting the constraint. sonar-command.test.ts: dropped "(CLI-1086)" from two test names. No sibling test in that block references a ticket, and the key tells a future reader nothing the name does not already say. eslint.config.js: comment restructured to lead with the two constraints that actually shape the block, and the files array now lists one path per line like the sibling block twenty lines below instead of one 103-column line. Verified the scoped rule still fires by planting an unconsumed Result in a covered file, and re-ran the integration spec for the exit-code contract.
…opagate-result-list-projects
|
Code Review ✅ Approved 6 resolved / 6 findingsConverts the ✅ 6 resolved✅ Quality: isResult() doc says orThrow(), the only caller must use match()
✅ Quality: Config comment mislabels orThrow() sites as "unconsumed Results"
✅ Quality: New negated toBeErrWith() message branch has no test
✅ Quality: PR description states a return type the code no longer has
✅ Bug: Matcher messages ignore this.isNot, so .not failures print nonsense
...and 1 more resolved from earlier reviews Implementation Status ✅ 2 of 2 objectives covered✅ CLI-1086 - 2 of 2 objectives coveredThis PR covers propagating the Result through command handlers and adding custom test matchers for Result types. ✅ 2 covered here
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
TL;DR
One command (
sonar list projects) converted to the Result-propagation shape CLI-1086 describes, so the shape can be argued over before the other ~40 handlers follow it. No behaviour change: exit codes stay 2 for a bad option and 1 for an API failure, both asserted by the integration spec. Also adds thetoBeOkWith()/toBeErrWith()matchers the ticket asks for, used by this command's own tests.What to challenge
The handler's error channel is
Error, not a narrower union and notCliError. No caller discriminates the members: the only consumer isauthenticatedAction(), andrunCommand()already maps any error to a message, an exit code and a hint. An earlier revision of this branch wrapped the domain failure in aCommandFailedErrorso the signature could readCliError; that wrap turned out to be a runtime no-op, and would have cost a ceremonial line in each of the other ~40 handlers.The rail collapses in
authenticatedAction(), not inrunCommand()as the ticket sketches. It is the smaller step, and it stays reversible once more commands are converted.Already settled, no need to re-raise
.orThrow()-based test assertions are deliberately not migrated. The ticket treats that as gradual.neverthrow/must-use-resultis scoped to the three converted files rather than repo-wide: the rule cannot recognise this repo's ownorThrow()as consuming a Result, so a wider scope would report correct code as broken. The reasoning lives ineslint.config.jsnext to the rule.