refactor: unify result types with discriminated Result<T, E> union#1125
Open
Hweinstock wants to merge 1 commit intomainfrom
Open
refactor: unify result types with discriminated Result<T, E> union#1125Hweinstock wants to merge 1 commit intomainfrom
Hweinstock wants to merge 1 commit intomainfrom
Conversation
6cde6f8 to
cc14f84
Compare
cc14f84 to
2a57e34
Compare
2a57e34 to
db3248f
Compare
db3248f to
a17bbae
Compare
a17bbae to
9cb5022
Compare
9cb5022 to
6642045
Compare
6642045 to
25dd47a
Compare
25dd47a to
ec8b476
Compare
ee1d76e to
0ec9e0f
Compare
0ec9e0f to
49f1732
Compare
49f1732 to
ce554da
Compare
ce554da to
cd560e1
Compare
cd560e1 to
450861e
Compare
450861e to
b6de8aa
Compare
b6de8aa to
352f76a
Compare
352f76a to
29296d0
Compare
29296d0 to
8cc8d41
Compare
Contributor
Coverage Report
|
Introduce a shared Result<T, E> type (inspired by Rust's Result) that
replaces ad-hoc { success: boolean; error?: string } patterns across
the codebase.
Key changes:
- Add src/lib/types.ts with Result<T, E> discriminated union type
- Add toError() helper in src/cli/errors.ts for catch blocks
- Migrate all command, operation, and primitive result types to Result<T>
- Error field is now Error (not string) on the failure branch
- Data fields only exist on the success branch (proper narrowing)
- Update all consumers to narrow before accessing branch-specific fields
- Update test assertions to match new Error objects and add narrowing
Contributor
Bug Bash Report — refactor: unify result types with discriminated Result<T, E> unionBranch: TL;DRNo in-scope blockers, highs, or mediums found. Refactor is behavior-preserving: every happy/error path exercised produces identical output to In-scope bugsNone. Pre-existing bugs (reproduce identically on
|
| Surface | Tested | Result |
|---|---|---|
| Build | yes | pass |
| CLI happy path (create/validate/status) | yes | pass |
| Error paths (missing project, malformed JSON, bad schema) | yes | pass (behavior identical to base) |
| Schema validation | yes | pass |
Removal paths (remove agent, remove all, nonexistent) |
yes | pass |
| Partition correctness | n/a | PR does not touch ARN/endpoint code |
Refactor LGTM from a black-box behavioral standpoint — every tested surface produces identical stdout/stderr/exit code to main.
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
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.
Description
Problem
The codebase uses an inconsistent result-like type that changes depending on the context. The implementations vary slightly, and are not explicit about (usually) sharing a common shape. This common shape also strips error types, and distills their information down to a string, which leads to brittle behavior when catching errors.
As an example, systems that span modules (ex. telemetry) must handle each result type individually, resulting in excessive branching and complexity. Additionally, loss of error information, means loss of this information in telemetry, making it difficult to impossible to tell the type of error, and if it was user/application caused.
Solution
instanceofchecking and error name matching for stronger exception handling and telemetry.Future Work
There are other result types that could be generalized with further work to support spread operators on the error as well. I left this OOS since the PR is already very large. Some other small patterns that this PR continues, but I think should change in the future:
mapResultfunction that allows data transformations with clear error propagation. This could avoid some nesting of try/catches in a few places.Related Issue
Closes #
Documentation PR
N/A — internal refactor, no user-facing changes.
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.