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

enhancement: include HTTP status text in error when response is unexpected #933

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Mar 19, 2024

Minor improvement, but adds a small amount of context mainly useful in the case that the API returns no error text.

@manicminer manicminer added the enhancement New feature or request label Mar 19, 2024
@manicminer manicminer requested a review from a team as a code owner March 19, 2024 19:06
@github-actions github-actions bot added the release-once-merged The SDK should be released once this PR is merged label Mar 19, 2024
@manicminer manicminer force-pushed the enhancement/http-status-text-in-error branch from 50a5c32 to 7708940 Compare March 19, 2024 19:07
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

One comment but otherwise LGTM 👍

// Note that the odata argument here is a best-effort and may be nil
if f := req.ValidStatusFunc; f != nil && f(resp.Response, resp.OData) {
return resp, nil
}

status := fmt.Sprintf("%d", resp.StatusCode)
if statusText := http.StatusText(resp.StatusCode); statusText != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Azure doesn't always use the Status Code to mean what the HTTP status means, but the HTTP Status Text is returned along side this, so we should be able to make this:

Suggested change
if statusText := http.StatusText(resp.StatusCode); statusText != "" {
if statusText := resp.Status; statusText != nil {

@manicminer manicminer force-pushed the enhancement/http-status-text-in-error branch from 7708940 to 0f5aa12 Compare March 20, 2024 13:32
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff tombuildsstuff merged commit bdede9b into main Mar 20, 2024
5 checks passed
@tombuildsstuff tombuildsstuff deleted the enhancement/http-status-text-in-error branch March 20, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants