Skip to content
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

Conversation

vvatanabe
Copy link
Collaborator

http.Request creation and sending were duplicated, so they have been aggregated into a common function.

  • RequestBuilder#Build function calls were aggregated into the Client#newRequest function.
  • All public functions build http.Request in the Client#newRequest function.
  • The sending of http.Request is now aggregated into the following three functions.
    • Client#sendRequest function: marshalling the response into a structure
    • Client#sendRequestRaw function: returns the Body of the response as is
    • Client#sendRequestStream function: use generics to specify a streamable type.

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #395 (57b824b) into master (b095938) will increase coverage by 1.69%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
audio.go 88.15% <100.00%> (-0.16%) ⬇️
chat.go 100.00% <100.00%> (ø)
chat_stream.go 100.00% <100.00%> (+10.71%) ⬆️
client.go 100.00% <100.00%> (+5.40%) ⬆️
completion.go 100.00% <100.00%> (ø)
edits.go 100.00% <100.00%> (ø)
embeddings.go 100.00% <100.00%> (ø)
engines.go 100.00% <100.00%> (ø)
files.go 94.73% <100.00%> (-0.92%) ⬇️
fine_tunes.go 100.00% <100.00%> (ø)
... and 4 more

... and 2 files with indirect coverage changes

@vvatanabe
Copy link
Collaborator Author

@sashabaranov Please check it.

@vvatanabe vvatanabe changed the title refactoring http request creation and sending refactor: refactoring http request creation and sending Jun 21, 2023
@sashabaranov
Copy link
Owner

This is super cool! 🚀

Comment on lines 24 to 25
func (b *HTTPRequestBuilder) Build(ctx context.Context,
method, url string, body any, header http.Header) (req *http.Request, err error) {
Copy link
Owner

@sashabaranov sashabaranov Jun 21, 2023

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) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sashabaranov

one-argument-per-line style

Looks Good! I will update to this style.

Copy link
Collaborator Author

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)
Copy link
Owner

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! 😄

Copy link
Collaborator Author

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?

Copy link
Owner

@sashabaranov sashabaranov left a 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
Copy link
Collaborator Author

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)

@sashabaranov sashabaranov merged commit f1b6696 into sashabaranov:master Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants