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

Refactoring SendGridClient to support inject external managed HttpClient #839

Merged
merged 1 commit into from
Jul 4, 2019
Merged

Refactoring SendGridClient to support inject external managed HttpClient #839

merged 1 commit into from
Jul 4, 2019

Conversation

akunzai
Copy link
Contributor

@akunzai akunzai commented Nov 29, 2018

Fixes #670

Fixes #897

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Remove HTTP Client Initialization
  • Setup each HTTP request with SendGrid client configuration
  • Cleanup constructors
  • Prefer options field over Properties

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 29, 2018
@SendGridDX
Copy link

SendGridDX commented Nov 29, 2018

CLA assistant check
All committers have signed the CLA.

@akunzai akunzai changed the title Refactoring SendGridClient to make it compatible with HttpClientFactory Refactoring SendGridClient to support inject external managed HttpClient Nov 30, 2018
@aevitas
Copy link
Contributor

aevitas commented Jun 23, 2019

LGTM. This is one of the preconditions to having proper support for ASP.NET Core's DI paradigm, as you'd ideally have a single HttpClient instance whose lifetime is managed by the container.

@thinkingserious Is there a reason this PR has been open for such a long period of time?

@akunzai
Copy link
Contributor Author

akunzai commented Jun 24, 2019

conflicts solved for Twilio branding updates

@thinkingserious
Copy link
Contributor

Thanks for helping out with a code review once again @aevitas! We appreciate it!

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty type: community enhancement feature request not on Twilio's roadmap labels Jul 4, 2019
@thinkingserious thinkingserious merged commit d6c2349 into sendgrid:master Jul 4, 2019
@thinkingserious
Copy link
Contributor

Hello @akunzai,

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

@akunzai akunzai deleted the http-client-factory-friendly branch July 4, 2019 05:07
childish-sambino pushed a commit that referenced this pull request May 14, 2020
… its reliability settings

Fixes #1000
Relates to #839

When the default client options were made static and the constructor was removed, there was no way to modify the reliability settings used by the retry handler. This fix adds back the constructor so clients can now be constructed with just the client options and the options will be used when creating a retry handled, if any.
childish-sambino pushed a commit that referenced this pull request May 14, 2020
… its reliability settings

Fixes #1000
Relates to #839

When the default client options were made static and the constructor was removed, there was no way to modify the reliability settings used by the retry handler. This fix adds back the constructor so clients can now be constructed with just the client options and the options will be used when creating a retry handler, if any.
childish-sambino pushed a commit that referenced this pull request May 14, 2020
…e its reliability settings

Fixes #1000
Relates to #839

When the default client options were made static and the constructors later removed, there was no way to modify the reliability settings used by the retry handler. This fix adds back the constructors so clients can now be constructed with just the client options and the options will be used when creating a retry handler, if any.
childish-sambino pushed a commit that referenced this pull request May 14, 2020
…e its reliability settings

Fixes #1000
Relates to #839

When the default client options were made static and the constructors later removed, there was no way to modify the reliability settings used by the retry handler. This fix adds back the constructors so clients can now be constructed with just the client options and the options will be used when creating a retry handler, if any.
childish-sambino pushed a commit that referenced this pull request May 19, 2020
…e its reliability settings (#1001)

Fixes #1000
Relates to #839

When the default client options were made static and the constructors later removed, there was no way to modify the reliability settings used by the retry handler. This fix adds back the constructors so clients can now be constructed with just the client options and the options will be used when creating a retry handler, if any.
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 type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient should be a static instance, and threadpool expired periodically SendgridClient constructor issue
4 participants