-
-
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
refactor: refactoring http request creation and sending #395
refactor: refactoring http request creation and sending #395
Conversation
Codecov Report
@@ Coverage Diff @@
## master #395 +/- ##
==========================================
+ Coverage 95.26% 96.95% +1.69%
==========================================
Files 17 17
Lines 676 690 +14
==========================================
+ Hits 644 669 +25
+ Misses 22 15 -7
+ Partials 10 6 -4
|
@sashabaranov Please check it. |
This is super cool! 🚀 |
internal/request_builder.go
Outdated
func (b *HTTPRequestBuilder) Build(ctx context.Context, | ||
method, url string, body any, header http.Header) (req *http.Request, err error) { |
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.
nit: maybe let's go one-argument-per-line style here, it's a bit longer but seems cleaner. What do you think?
func (b *HTTPRequestBuilder) Build(
ctx context.Context,
method string,
url string,
body any,
header http.Header
) (req *http.Request, err error) {
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.
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.
DONE!
time.Sleep(10 * time.Nanosecond) | ||
}) | ||
ctx := context.Background() | ||
ctx, cancel := context.WithTimeout(ctx, time.Nanosecond) |
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.
Wow, that's some bleeding edge timeout! 😄
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 didn't want to spend time running tests.;D
Here is some background on this timeout test.
sendRequest
, sendRequestRaw
, and sendRequestStream
each exec c.config.HTTPClient.Do(req)
internally. This error handling was not covered test. That is why we implemented a workaround for c.config.HTTPClient.Do(req)
with a timeout, to intentionally cause it to return an error.
It's hard to convey the intent of the test, isn't it? Would it be better to write a test that executes sendRequest
, sendRequestRaw
, and sendRequestStream
directly?
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.
This is a really beautiful PR!
@@ -55,8 +92,6 @@ func (c *Client) sendRequest(req *http.Request, v any) error { | |||
req.Header.Set("Content-Type", "application/json; charset=utf-8") | |||
} | |||
|
|||
c.setCommonHeaders(req) | |||
|
|||
res, err := c.config.HTTPClient.Do(req) | |||
if err != nil { | |||
return err |
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 wanted to cover the above line.
#395 (comment)
http.Request
creation and sending were duplicated, so they have been aggregated into a common function.RequestBuilder#Build
function calls were aggregated into theClient#newRequest
function.http.Request
in theClient#newRequest
function.http.Request
is now aggregated into the following three functions.Client#sendRequest
function: marshalling the response into a structureClient#sendRequestRaw
function: returns the Body of the response as isClient#sendRequestStream
function: use generics to specify astreamable
type.