search

pointfreeco / swift-clocks

⏰ A few clocks that make working with Swift concurrency more testable and more versatile.
MIT License
266 stars 17 forks source link

Sort TestClock suspensions on insert rather than in the advance method #8

Closed danieleformichelli closed 1 year ago

danieleformichelli commented 1 year ago

Fixes https://github.com/pointfreeco/swift-clocks/discussions/7

Looking better at it, looks like the implementation was technically working, because after getting the first suspension, then it was using advance, so the suspensions were sorted there.

Nevertheless, I think this approach is clearer and safer, so I though it would have been better to open the PR anyway

mbrandonw commented 1 year ago

Hey @danyf90, yeah after looking into this a bit more this morning and trying to get a failing test I came to the same conclusion. It does work as-is.

I'm not sure this approach is necessarily safer or cleaner though. The main point of run is to loop while there is any work in the suspensions queue, and we can pick any element in the array to advance to. We could pick the last or even a random one, and it would all work the same.

I do think, however, that we should definitely get test coverage on this. Would you like to revert your changes and write a test? Or I can do it if you don't have time.

danieleformichelli commented 1 year ago

Hey @mbrandonw, makes sense, I'll close the PR for now. Feel free to add the test if you have the time. 🙏

Just wanted to add that I think that if 3 of us misinterpreted the code, then it's probably an indication that the current implementation is not super clear.