Skip to content

feat: do not close or manage lifecycle of http-client passed in #25

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 31, 2017
Merged

feat: do not close or manage lifecycle of http-client passed in #25

merged 4 commits into from
Oct 31, 2017

Conversation

maxxedev
Copy link
Contributor

@maxxedev maxxedev commented Oct 4, 2017

No description provided.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 4, 2017
@SendGridDX
Copy link

SendGridDX commented Oct 4, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mbernier mbernier added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 11, 2017
andy-trimble
andy-trimble suggested changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@andy-trimble andy-trimble left a comment

Choose a reason for hiding this comment

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

Please add documentation to the Client(CloseableHttpClient httpClient) constructor indicating that the client will not be closed on finalization.

src/main/java/com/sendgrid/Client.java
@@ -48,13 +48,15 @@ public HttpDeleteWithBody(final String uri) {

private CloseableHttpClient httpClient;
private Boolean test;
private boolean createdHttpClient;
Copy link
Contributor

Choose a reason for hiding this comment

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

Set default value to false

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard this comment. The default value will be false. That is sufficient.

@andy-trimble
Copy link
Contributor

LGTM

@maxxedev
Copy link
Contributor Author

could this be merged please?

@thinkingserious
Copy link
Contributor

@maxxedev,

LGTM

Please fix the conflict and I'll merge this :)

Thanks!

With Best Regards,

Elmer

@maxxedev
Copy link
Contributor Author

ah conflicts.
fixed now.

@thinkingserious thinkingserious merged commit 49a12eb into sendgrid:master Oct 31, 2017
@thinkingserious
Copy link
Contributor

Hello @maxxedev,

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

@eshanholtz eshanholtz changed the title do not close or manage lifecycle of http-client passed in fix: do not close or manage lifecycle of http-client passed in Jan 31, 2020
@eshanholtz eshanholtz changed the title fix: do not close or manage lifecycle of http-client passed in feat: do not close or manage lifecycle of http-client passed in Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-trimble andy-trimble andy-trimble approved these changes

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maxxedev @SendGridDX @andy-trimble @thinkingserious @mbernier @apigirl