#405 reuse http client #417
Conversation
thinkingserious
added
the
status: code review request
src/SendGrid/SendGridClient.cs
Outdated
| { | ||
| HttpResponseMessage response = await client.SendAsync(request, cancellationToken).ConfigureAwait(false); | ||
| HttpResponseMessage response = await client.SendAsync(request, cancellationToken); |
There was a problem hiding this comment.
Why drop the .ConfigureAwait(false);? I believe we had async issues before without this call. Please see this thread: #362
|
Done. Learned a new thing now about why use ConfigureAwait(false) in libraries. |
src/SendGrid/SendGridClient.cs
Outdated
| /// <returns>Interface to the SendGrid REST API</returns> | ||
| public SendGridClient(IWebProxy webProxy, string apiKey, string host = "https://api.sendgrid.com", Dictionary<string, string> requestHeaders = null, string version = "v3", string urlPath = null) | ||
| : this(apiKey, host, requestHeaders, version, urlPath) | ||
| public SendGridClient(IWebProxy webProxy, string apiKey, string host = null, Dictionary<string, string> requestHeaders = null, string version = "v3") |
There was a problem hiding this comment.
Please re-add the urlPath parameter so we can avoid a breaking change. It allows for the user to specify a default endpoint path.
src/SendGrid/SendGridClient.cs
Outdated
| /// <returns>Interface to the SendGrid REST API</returns> | ||
| public SendGridClient(string apiKey, string host = null, Dictionary<string, string> requestHeaders = null, string version = "v3", string urlPath = null) | ||
| public SendGridClient(string apiKey, string host = null, Dictionary<string, string> requestHeaders = null, string version = "v3") |
There was a problem hiding this comment.
Please re-add the urlPath parameter so we can avoid a breaking change. It allows for the user to specify a default endpoint path.
src/SendGrid/SendGridClient.cs
Outdated
| /// The web proxy. | ||
| /// </summary> | ||
| private IWebProxy webProxy; | ||
| private readonly string version; |
There was a problem hiding this comment.
Please include summary comments.
Added summary comments
|
Done. |
| @@ -63,10 +52,67 @@ public class SendGridClient | |||
| /// <param name="version">API version, override AddVersion to customize</param> | |||
| /// <param name="urlPath">Path to endpoint (e.g. /path/to/endpoint)</param> | |||
| /// <returns>Interface to the SendGrid REST API</returns> | |||
| public SendGridClient(IWebProxy webProxy, string apiKey, string host = "https://api.sendgrid.com", Dictionary<string, string> requestHeaders = null, string version = "v3", string urlPath = null) | |||
| : this(apiKey, host, requestHeaders, version, urlPath) | |||
There was a problem hiding this comment.
Not sure why this line was removed.
| @@ -79,22 +125,8 @@ public SendGridClient(IWebProxy webProxy, string apiKey, string host = "https:// | |||
| /// <param name="urlPath">Path to endpoint (e.g. /path/to/endpoint)</param> | |||
| /// <returns>Interface to the SendGrid REST API</returns> | |||
| public SendGridClient(string apiKey, string host = null, Dictionary<string, string> requestHeaders = null, string version = "v3", string urlPath = null) | |||
| : this(null, apiKey, host, requestHeaders, version, urlPath) | |||
There was a problem hiding this comment.
.. and then this line added here.
|
New version create the HttpClient in ctor and this is done in the ctor that accepts all parameters (WebProxy). Previous version of this ctor would call the other ctor (without the webproxy parameter) and then just set the WebPoxy property. |
|
That makes sense, and thanks for responding so quickly. My main concern is that this may cause a breaking change because the function signatures are no longer the same. I will double check the tests and make sure code written against the previous versions of those constructors do not break. |
|
Thanks again for the awesome PR! Please take a moment to fill out this form so that we can send you some swag :) |
Changed SendGridClient to keep/reuse HttpClient instance
Removed obsolete properties and parameters (that cannot be re-set on the HttpClient)
Modified unit tests to pass custom headers to SendGridClient constructor
Change in Examples to avoid exception when there is no groups and correct text message.