-
-
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
fix: fullURL endpoint generation #817
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #817 +/- ##
==========================================
+ Coverage 98.46% 99.00% +0.54%
==========================================
Files 24 26 +2
Lines 1364 1410 +46
==========================================
+ Hits 1343 1396 +53
+ Misses 15 8 -7
Partials 6 6 ☔ View full report in Codecov by Sentry. |
great stuff! thank you so much for the PR and sorry for the long review |
edits.go
Outdated
@@ -38,7 +38,10 @@ will need to migrate to GPT-3.5 Turbo by January 4, 2024. | |||
You can use CreateChatCompletion or CreateChatCompletionStream instead. | |||
*/ | |||
func (c *Client) Edits(ctx context.Context, request EditsRequest) (response EditsResponse, err error) { | |||
req, err := c.newRequest(ctx, http.MethodPost, c.fullURL("/edits", fmt.Sprint(request.Model)), withBody(request)) | |||
req, err := c.newRequest(ctx, http.MethodPost, c.fullURL( |
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: could we please make it one argument per line?
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.
(applies to some other files as well)
if containsSubstr(azureDeploymentsEndpoints, suffix) { | ||
baseURL = fmt.Sprintf("%s/%s/%s", baseURL, azureDeploymentsPrefix, azureDeploymentName) | ||
} | ||
} |
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 part is pretty hard to figure about 🤔 Could I suggest breaking it down into more functions and keeping fullURLOptions
closer to the code that uses it?
Something like this:
func (c *Client) suffixWithAPIVersion(previousSuffix string) (newSuffix string) {
parsedSuffix, err := url.Parse(previousSuffix)
if err != nil {
panic("failed to parse url suffix")
}
query := parsedSuffix.Query()
query.Add("api-version", c.config.APIVersion)
return fmt.Sprintf("%s?%s", parsedSuffix.Path, query.Encode())
}
func (c *Client) baseURLWithAzureDeployment(baseURL, suffix, model string) (newBaseURL string) {
azureDeploymentName := c.config.GetAzureDeploymentByModel(model)
if azureDeploymentName == "" {
azureDeploymentName = "UNKNOWN"
}
baseURL = fmt.Sprintf("%s/%s", baseURL, azureAPIPrefix)
if containsSubstr(azureDeploymentsEndpoints, suffix) {
baseURL = fmt.Sprintf("%s/%s/%s", baseURL, azureDeploymentsPrefix, azureDeploymentName)
}
return baseURL
}
// fullURL returns full URL for request.
func (c *Client) fullURL(suffix string, setters ...fullURLOption) string {
baseURL := strings.TrimRight(c.config.BaseURL, "/")
urlOptions := fullURLOptions{}
for _, setter := range setters {
setter(&args)
}
if c.config.APIType == APITypeAzure || c.config.APIType == APITypeAzureAD {
baseURL = baseURLWithAzureDeployment(baseURL, suffix, urlOptions.model)
}
if c.config.APIVersion != "" {
suffix = c.suffixWithAPIVersion(suffix)
}
return fmt.Sprintf("%s%s", baseURL, suffix)
}
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've completed the suggested changes. Could you please review it again?
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 work, thank you!
fix: generation azure fullURL #794
feat: Use setters pattern for fullURL method #636