-
Notifications
You must be signed in to change notification settings - Fork 4
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
Throttle refactor #157
Throttle refactor #157
Conversation
CI failure was unrelated to change:
|
First off, thank you so much for the PR. This is sorely needed. It's going to take me a while to review since it's a nice and complicated operator. I'll also want to sanity check it against some Combine output in a playground. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put it through its paces and compared 1:1 with Combine. So far the same elements are emitted in the same order. These timing type operators are always tricky. However, the test suite is robust. I think we're good to proceed!
Description
Refactors the throttle operation to behave more similarly to Combine's.
Also improves the logic / performance of the operator by using fewer Tasks.
Note: while swift-async-algorithms does have their own throttle operator, there has been much discussion around undesirable behavior of the operator. See this Swift forums thread and this GitHub issue. I've mostly ignored the design of swift-async-algorithms' implementation in favor of attempting to replicate Combine's.
I've also improved the throttle DSL tests. There was a decent amount of logic attempting to "figure out" what the elements should be at a given step, which complicated the test harness and was prone to introducing errors itself. I've chosen to adjust the DSL to be more similar to the validation used by swift-async-algorithms, which uses alignment to determine what elements should be present at each validation point.
Checks:
ThrottleSequenceTests
x1000