maintain underlying error structs to allow for type conversion #293
Conversation
defensive error checking
Codecov Report
@@ Coverage Diff @@
## master #293 +/- ##
==========================================
+ Coverage 92.43% 92.96% +0.52%
==========================================
Files 22 22
Lines 661 668 +7
==========================================
+ Hits 611 621 +10
+ Misses 36 35 -1
+ Partials 14 12 -2
|
|
@qhenkart thank you for the PR! It's a bit hard for me to follow all the error cases here — could you please either add a test or code sample clearly demonstrating the benefits of this approach? |
client.go
Outdated
| HTTPStatusCode: resp.StatusCode, | ||
| Err: err, | ||
| } | ||
| if errRes.Error != nil { | ||
| reqErr.Err = errRes.Error | ||
| reqErr.Err = fmt.Errorf(errRes.Error.Message) |
There was a problem hiding this comment.
Wouldn't we lose the ability to use errors.Is and errors.As here? https://pkg.go.dev/errors
There was a problem hiding this comment.
you are right. I have updated the code and the error message to cover this case and still match the expected error result illustrated in the existing tests
|
@sashabaranov I have added a test to ensure type conversion as well as updated the README as requested in the linked issue to provide guidance on how to handle errors utilizing proper error package syntax. |
README.md
Outdated
|
|
||
| Other examples: | ||
| Open-AI maintains clear documentation on how to [handle API errors](https://platform.openai.com/docs/guides/error-codes/api-errors) |
There was a problem hiding this comment.
Could we please wrap this into <details> tag as it is done below?
| errRes.Error.HTTPStatusCode = resp.StatusCode | ||
| return fmt.Errorf("error, status code: %d, message: %w", resp.StatusCode, errRes.Error) |
There was a problem hiding this comment.
I wonder if the previous version was working with the errors.As due to %w usage
There was a problem hiding this comment.
yes you are right. I ran into the issue because I was using type conversion instead of errors.As. I suppose the extra information in the documentation would have negated the need for this PR. However I still think it is an improvement for the error type to handle its own messaging and formatting. Also it doesn't hurt to also allow type conversion even if it is no longer the idiomatic methodology
README.md
Outdated
|
|
||
| example: | ||
| ``` | ||
| e := &APIError{} |
There was a problem hiding this comment.
I guess it should be openai.APIError
Resolves #292, #288
Maintains underlying error structs to allow consumers to utilize type conversion and create a defensive structure as defined by the Open AI error handling documentation.
It also slightly modifies the Error() functions to that they are responsible for converting the struct into an informational string versus just a fragment of its parent
Modifies only a single test that was missing a colon when all of the other tests have colons (including the RequestError Error function that was missing the colon that the other Error function included