-
Notifications
You must be signed in to change notification settings - Fork 2
QueryToResults error handling #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: maxeng-error-handling-2
Are you sure you want to change the base?
Conversation
Signed-off-by: Max Englander <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves error handling by propagating errors from query-to-result conversion instead of panicking, ensuring that errors are properly surfaced in both tests and runtime.
- Update QueryResultToRecords, parseValue, leadDecimalWithZero, and formatISO8601 to return errors.
- Add error assertions in tests and update sync handling in the Planetscale Edge Database integration.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
cmd/internal/types_test.go | Tests now assert no errors when calling QueryResultToRecords. |
cmd/internal/types.go | Refactors functions to return errors instead of panicking on failure. |
cmd/internal/planetscale_edge_database.go | Handles errors from QueryResultToRecords in the sync process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
panic(fmt.Sprintf("unexpected query.Type: %#v", queryColumnType)) | ||
return Value{}, fmt.Errorf("unexpected query.Type: %#v", queryColumnType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an improvement to be sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i put the panic there originally jfyi 😅
Prev: #136
Surface errors from converting queries to results.
Next: #138