Skip to content

Added a warning about the error return from sendgrid.API#143

Merged
thinkingserious merged 4 commits intosendgrid:masterfrom
leandro-lugaresi:issue99
Oct 10, 2017
Merged

Added a warning about the error return from sendgrid.API#143
thinkingserious merged 4 commits intosendgrid:masterfrom
leandro-lugaresi:issue99

Conversation

@leandro-lugaresi
Copy link
Copy Markdown
Contributor

The solution for the issue #99 was this warning because the sendgrid.API is a generic function and should not handle this problem.
closes #99

The solution for the issue sendgrid#99 was this warning because the sendgrid.API is a generic function and should not handle this problem.
closes sendgrid#99
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 6, 2017
@tariq1890
Copy link
Copy Markdown
Contributor

tariq1890 commented Oct 9, 2017

@leandro-lugaresi This still returns an error object. You might need to have this instead of fmt.Errorf

log.Printf("api response: HTTP %d: %s", resp.StatusCode, resp.Body)

Also, if you guys are open to using a logging libraries, we can use them to create warning statements.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 9, 2017
@leandro-lugaresi
Copy link
Copy Markdown
Contributor Author

@tariq1890 but this code represent the library use, for me make sense return an error in most cases.
WDYT of putting a comment with one disclaimer about log or create an error?

@tariq1890
Copy link
Copy Markdown
Contributor

I feel an error should be returned only if the code failed to send a request or receive a response. If we get a 400 response, it shows that the request/response processing was done successfully.

@leandro-lugaresi
Copy link
Copy Markdown
Contributor Author

🤔
The request represents one action and if the response is 400 Bad request for example the request failed and the action is not done. The application must handle this (log, return the error to the user...)

 resp, err := sendgrid.API(request)
 if err != nil {
 	return err
 }
 if resp.StatusCode >= 400 {
 	// something goes wrong and you have to handle (return an error or log the problem)
	// See https://sendgrid.com/docs/API_Reference/Web_API_v3/How_To_Use_The_Web_API_v3/responses.html#-Status-Codes
 	log.Printf("api response: HTTP %d: %s", resp.StatusCode, resp.Body)
 	return nil
 }

@thinkingserious
Copy link
Copy Markdown
Contributor

Thanks for the thoughtful back and forth :)

I think the purpose of this PR is to provide a suggested means of handling non 2xx responses. Perhaps we provide both means of handling the non 2xx response with an explanation for each case.

@tariq1890
Copy link
Copy Markdown
Contributor

tariq1890 commented Oct 9, 2017

@leandro-lugaresi I ask that you think of this in the API client perspective. 400 is returned by an API when a request is not constructed correctly. So this is the expected outcome and the appropriate response has been returned successfully. We return an err if something unexpected happens in the functioning of the API client. Here the API client is working as expected.

It is up to the user who makes the API calls to handle these expected 400 responses however he/she may deem fit. The API client has done its job successfully by sending the request over and getting the response back to the caller. My 2 cents :)

@thinkingserious
Copy link
Copy Markdown
Contributor

That's a great articulation of your point @tariq1890.

I agree. This PR's purpose is to provide a generic example of how to handle those non-2xx responses.

@leandro-lugaresi
Copy link
Copy Markdown
Contributor Author

@thinkingserious So WDYT about the snippet on the previous comment?
#143 (comment)

@thinkingserious
Copy link
Copy Markdown
Contributor

@leandro-lugaresi,

I think you should incorporate @tariq1890's feedback in your example. I think you should demonstrate both approaches.

@leandro-lugaresi
Copy link
Copy Markdown
Contributor Author

  • text explaining the behavior for non-2xx status code
  • show the log option
  • show the return option

@thinkingserious thinkingserious merged commit f8c54ec into sendgrid:master Oct 10, 2017
@thinkingserious
Copy link
Copy Markdown
Contributor

Hello @leandro-lugaresi,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sendgrid.API() isn't returning error properly

3 participants