{types,graphql_test}: Adds Result.ErrorsJoined implementation.#717
{types,graphql_test}: Adds Result.ErrorsJoined implementation.#717chris-ramon wants to merge 12 commits intomasterfrom
Result.ErrorsJoined implementation.#717Conversation
|
I'm getting to work. See my steps and track my cost here.
|
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
types.go
Outdated
| // ErrorsJoined joins and returns the result errors. | ||
| func (r *Result) ErrorsJoined() error { | ||
| if r.Errors == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var result error | ||
| for _, err := range r.Errors { | ||
| if result == nil { | ||
| result = fmt.Errorf("%w", err) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| result = fmt.Errorf("%w: %w", err, result) | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
I noticed that the errors are being joined in reverse order (the first error in the slice ends up at the end of the error message). Is this intentional?
Also, it might be worth adding a bit more documentation about the behavior, particularly that:
- It returns nil if there are no errors
- The error wrapping preserves the original error types (so
errors.Iswould work with the wrapped errors) - The formatting of the joined error (with
:as separator)
There was a problem hiding this comment.
I noticed that the errors are being joined in reverse order (the first error in the slice ends up at the end of the error message). Is this intentional?
Yes because we want to show the last error first, similar how stack traces work in order to be aware what happened with the program bottom-up.
Also, it might be worth adding a bit more documentation about the behavior, particularly that:
- It returns nil if there are no errors
- The error wrapping preserves the original error types (so
errors.Iswould work with the wrapped errors)- The formatting of the joined error (with
:as separator)
Ok sounds good, answering inline:
- It returns nil if there are no errors
Great call, but it is redundant to add it as comment since the zero value of error is nil and is common understanding that behavior therefore not added as a part of the code comments.
- The error wrapping preserves the original error types (so
errors.Iswould work with the wrapped errors)
Ok, that's a great suggestion, for the test, on the implementation side there's no use case for leveraging errors.Is, I went ahead and updated it in the following git commit: 9bd0b67
- The formatting of the joined error (with
:as separator)
Ok, that's great suggestion as well, I went ahead and added it as part of the code comment, added via the git commit: b2f18a1
graphql_test.go
Outdated
| expected := errors.New("second error: first error") | ||
|
|
||
| if err := r.ErrorsJoined(); err != nil { | ||
| if !reflect.DeepEqual(err.Error(), expected.Error()) { |
There was a problem hiding this comment.
Minor suggestion: When comparing error messages, you can directly compare the strings rather than using reflect.DeepEqual. It's simpler and more explicit about what you're comparing.
| if !reflect.DeepEqual(err.Error(), expected.Error()) { | |
| if err.Error() != expected.Error() { |
There was a problem hiding this comment.
Great call, addressed via the comment above: #717 (comment)
| func TestResultErrorsJoinedSuccess(t *testing.T) { | ||
| r := graphql.Result{ | ||
| Errors: []gqlerrors.FormattedError{ | ||
| {Message: "first error"}, | ||
| {Message: "second error"}, | ||
| }, | ||
| } | ||
|
|
||
| expected := errors.New("second error: first error") |
There was a problem hiding this comment.
It might be worth adding a test that verifies the error wrapping behavior works correctly with errors.Is or errors.As. This would ensure that the wrapping with %w is functioning as expected.
1 participant
Details
types: addsResult.ErrorsJoinedmethod.graphql_test: addsResult.ErrorsJoined100& test coverage unit tests..circleci/config.yml: upgradescirclecigo versions.Added in order to be able to join errors because currently
Result.Errorsreturns an slice of errors and is common use case to have it consolidated into a single error.This PR leverages the
%werror formatting verb in order to wrap errors.Ref: https://go.dev/doc/go1.13#error_wrapping
Test Plan
✔️ Tested that
Result.ErrorsJoinedworks as expected viago test -cover ...: