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

Batching #42

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vanelizarov
Copy link

@vanelizarov vanelizarov commented Nov 20, 2020

@vanelizarov vanelizarov mentioned this pull request Nov 20, 2020
@vanelizarov
Copy link
Author

@FZambia

@FZambia
Copy link
Member

FZambia commented Dec 18, 2020

@vanelizarov Hello!

I have some questions to this pr:

  1. There was a discussion in Make all async functions return a Future #31 - how this pr addresses my comments (specifically this one)
  2. Tests are not passing here.

Subscription batching is a good feature – but I can't start reviewing this in current pr form - it's too heavy and has non-obvious changes.

In general it's better to decouple different things to different pull requests. But before doing this let's elaborate more on my point 1 above.

@vanelizarov
Copy link
Author

vanelizarov commented Dec 19, 2020

@FZambia about #31 and your discussion with @synw, IMHO the only reason to have awaitable connect, disconnect, etc is what a developer expects functions/methods marked as async return - Future. I can't see another reason to have it, because, as you've said, connect and some other events may occur more than once during the client's lifecycle, so the awaitable methods are not completely necessary in this case. In the context of this PR, I can revert these changes back to the original and left this rework for the future

About tests, I've started rewriting them but at some point, I got stuck and put this activity on the back burner. I can push the changes, so you can review them in the current state. It would be also great if you tell me if I am doing it the right way or not.

About decoupling, I was thinking about it during the process but all in all, figured out that it would be more correct to suggest this feature as one PR because all its parts are tightly connected to each other. As I mentioned above, mostly it repeats the logic from the web library but adjusted for Dart language specificity, and the structure of this library. Also, I can provide some explanations if needed

@SimonVillage
Copy link
Contributor

Is this still considered to be merged at some point?

@FZambia
Copy link
Member

FZambia commented Oct 7, 2021

@SimonHausdorf hello, this pr was too heavy and contained some non-obvious changes, so I have not looked closely. If you are interested in having batching (are you? or you need sth else from this pr?) then better to start from discussing a use case and make a minimal improvement that will help your use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make all async functions return a Future
3 participants