-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
move error_accumulator into internal pkg (#304) #335
move error_accumulator into internal pkg (#304) #335
Conversation
Codecov Report
@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 92.45% 92.97% +0.51%
==========================================
Files 18 17 -1
Lines 636 626 -10
==========================================
- Hits 588 582 -6
+ Misses 35 32 -3
+ Partials 13 12 -1
|
@sashabaranov I moved error_accumulator into internal pkg. Please check this PR. |
} | ||
|
||
respErr = stream.unmarshalError() | ||
if respErr != nil { |
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.
All these tests might benefit from https://github.com/sashabaranov/go-openai/blob/master/internal/test/checks/checks.go
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.
@sashabaranov I see! I'll send you a PR to refactor the test in the near future.
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.
Great PR, thank you so much!
Moving error_accumulator to the internal package causes circular import. To avoid this, the following commit removes the ErrorResponse dependency from error_accumulator.
2c9d003
Next, the following commit moved error_accumulator to the internal package.
15653fd
And add a test for ErrTooManyEmptyStreamMessages in stream_reader. (Just increased test coverage.)
444c808