-
Notifications
You must be signed in to change notification settings - Fork 1.6k
En/http redirects #1640
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
base: development
Are you sure you want to change the base?
En/http redirects #1640
Conversation
@Umang01-hash , Can we also check and add tests for url with path and query params as well as request Bodies, headers etc.....get handled successfully through the redirect. I feel that is not the case right now. |
@Umang01-hash, Can we add test for these cases ? I feel this implementation can fall short in fitting these usecases. |
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.
@Umang01-hash cc @coolwednesday
I got not feedback on these
- En/http redirects #1640 (comment)
- En/http redirects #1640 (comment)
- En/http redirects #1640 (comment)
so I'm moving my comment to "request changes"
// NewRedirect creates a redirect response with the specified URL and status code. | ||
func NewRedirect(url string) Redirect { | ||
return Redirect{ | ||
URL: url, | ||
} | ||
} |
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 can be removed.
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.
Good catch
// NewRedirect creates a redirect response with the specified URL and status code. | ||
func NewRedirect(url string) Redirect { | ||
return Redirect{ | ||
URL: url, | ||
} | ||
} |
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.
Good catch
var resp any | ||
|
||
switch v := data.(type) { |
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.
Nitpicking, there are no need to update this, especially if we consider it diverges from Go standards
var resp any | |
switch v := data.(type) { | |
var resp any | |
switch v := data.(type) { |
case resTypes.Redirect: | ||
switch r.method { | ||
case http.MethodPost, http.MethodPut, http.MethodPatch: | ||
statusCode = http.StatusSeeOther // 303 | ||
default: | ||
statusCode = http.StatusFound // 302 by default | ||
} |
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.
🎨 style
case resTypes.Redirect: | |
switch r.method { | |
case http.MethodPost, http.MethodPut, http.MethodPatch: | |
statusCode = http.StatusSeeOther // 303 | |
default: | |
statusCode = http.StatusFound // 302 by default | |
} | |
case resTypes.Redirect: | |
// HTTP 302 by default | |
statusCode = http.StatusFound | |
switch r.method { | |
case http.MethodPost, http.MethodPut, http.MethodPatch: | |
// HTTP 303 | |
statusCode = http.StatusSeeOther | |
} |
case resTypes.Redirect: | ||
switch r.method { | ||
case http.MethodPost, http.MethodPut, http.MethodPatch: | ||
statusCode = http.StatusSeeOther // 303 | ||
default: | ||
statusCode = http.StatusFound // 302 by default | ||
} |
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 is something I cannot explain: why we computed this line 30
statusCode, errorObj := r.determineResponse(data, err)
And we here don't care about statusCode we computed
Shouldn't we check if data.type is not redirect before calling determineResponse ? And so redirect without calling it
Pull Request Template
Description:
Example:
Additional Information:
Checklist:
goimport
andgolangci-lint
.Thank you for your contribution!