Skip to content

Lazy Streaming Client #82

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

Closed
wants to merge 6 commits into from
Closed

Conversation

WyriHaximus
Copy link
Contributor

@WyriHaximus WyriHaximus commented Feb 9, 2019

Implements / Closes #80

This is in preparation of creating a FactoryLazyStreamingClientTest
for the new LazyStreamingClient
@WyriHaximus WyriHaximus mentioned this pull request Feb 9, 2019
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@WyriHaximus Thanks for looking into this, but it looks like this changeset might be incomplete, can you take a look at this? 👍

@WyriHaximus
Copy link
Contributor Author

@clue yeah working on that, hence the WIP in the title 😋 , was about the commit the LazyStreamingClient when I ran into a bug and had to redesign the entire class 🤐 .

@WyriHaximus WyriHaximus changed the title [WIP] Lazy Streaming Client Lazy Streaming Client Feb 10, 2019
@WyriHaximus
Copy link
Contributor Author

@clue this PR is ready for review

Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@WyriHaximus Thank you very much for this update! I will review this more thoroughly as soon as time permits.

Before getting this in, I would like to finish support for connection cancellation and timeout support. Your suggested changes make perfect sense in combination with this and I will report back with an update to take advantage of this with your changes combined 👍

@clue clue self-requested a review February 22, 2019 21:07
@clue clue added this to the v2.3.0 milestone Feb 22, 2019
@WyriHaximus
Copy link
Contributor Author

@clue cool keep me updated. Do you at some point also want to include a TTL for the connection where it will disconnect after a certain amount of time without activity? (Thinking ahead)

@WyriHaximus
Copy link
Contributor Author

@clue I'll update this PR later today 👍

@clue
Copy link
Owner

clue commented Mar 2, 2019

@WyriHaximus Just a quick progress update: I've picked this one up earlier this week and have worked out most of the outstanding issues already (including reconnection, idle timeouts and handling pubsub subscriptions). I'll finish this asap because I plan to start using this in a live project no later than next week :shipit:

@WyriHaximus
Copy link
Contributor Author

@clue ok cool, do you still want me to update this or should we close it instead then?

@clue
Copy link
Owner

clue commented Mar 2, 2019

@WyriHaximus If you feel an update provides any value, I welcome any contribution 👍 Other than that, I'd really want to avoid wasting anybody's work due to duplicate efforts. I'm currently planning to squash your changes to a single commit in a new PR and give proper credit for your initial work 👍

@WyriHaximus
Copy link
Contributor Author

@clue that works for me 👍

@clue
Copy link
Owner

clue commented Mar 8, 2019

See #87 :shipit: 🎉

@WyriHaximus
Copy link
Contributor Author

See #87 :shipit:

Awesome :shipit:

@clue clue closed this in #87 Mar 8, 2019
@clue clue removed this from the v2.3.0 milestone Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants