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

Throttle refactor #157

Merged
merged 10 commits into from
Nov 23, 2024
Merged

Throttle refactor #157

merged 10 commits into from
Nov 23, 2024

Conversation

aim2120
Copy link
Collaborator

@aim2120 aim2120 commented Nov 19, 2024

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:

  • Successfully ran ThrottleSequenceTests x1000

@aim2120 aim2120 marked this pull request as ready for review November 19, 2024 15:47
@aim2120
Copy link
Collaborator Author

aim2120 commented Nov 19, 2024

CI failure was unrelated to change:

✘ Test delay_DelaysCorrectlyEvenAfterIntervalHasPassed() recorded an issue at DelaySequenceTests.swift:118:25: Expectation failed: (elapsedTime → 0.02 seconds) >= (delayDuration + .milliseconds(15) → 0.025 seconds)

@Tyler-Keith-Thompson
Copy link
Owner

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.

Copy link
Owner

@Tyler-Keith-Thompson Tyler-Keith-Thompson left a 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!

@Tyler-Keith-Thompson Tyler-Keith-Thompson merged commit 929a9f9 into main Nov 23, 2024
2 checks passed
@Tyler-Keith-Thompson Tyler-Keith-Thompson deleted the throttle-refactor branch November 23, 2024 17:53
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.

2 participants