Skip to content
This repository was archived by the owner on Mar 17, 2025. It is now read-only.

FirebaseHttpClient: add forceReuse #260

Merged
merged 3 commits into from
Oct 27, 2017
Merged

FirebaseHttpClient: add forceReuse #260

merged 3 commits into from
Oct 27, 2017

Conversation

proppy
Copy link
Contributor

@proppy proppy commented Apr 10, 2017

Fixes #230 #286

@proppy proppy requested a review from ed7coyne April 10, 2017 09:02
class FirebaseHttpClientEsp8266 : public FirebaseHttpClient {
public:
FirebaseHttpClientEsp8266() {}

void setReuseConnection(bool reuse) override {
void setReuseConnection(bool reuse) override {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

trailing space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@ed7coyne ed7coyne left a comment

Choose a reason for hiding this comment

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

It feels a little dirty to just keep the client open but the end result is a pretty clean integration so LGTM.

class ForceReuseHTTPClient : public HTTPClient {
public:
void end() {
if(connected()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another possibility is to call HTTPClient::end here with _canReuse set to true before it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that better only because it deviates less from the base so as the base changes we can merge with impunity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@skhaz
Copy link

skhaz commented May 23, 2017

It worked for me

@proppy
Copy link
Contributor Author

proppy commented Oct 27, 2017

Merging: sorry all for the delay.

@proppy proppy merged commit c73ca0e into master Oct 27, 2017
@proppy proppy mentioned this pull request Oct 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants