fix: Client::has_pending_recoveries #4462
Conversation
earlier it returned true if client_recovery_progress_receiver returned empty btreemap
we are using it in fedi |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| @@ -1470,7 +1470,7 @@ impl Client { | |||
| .client_recovery_progress_receiver | |||
| .borrow() | |||
| .iter() | |||
| .any(|(_id, progress)| !progress.is_done()) | |||
| .all(|(_id, progress)| progress.is_done()) | |||
There was a problem hiding this comment.
The problem with this fn was that it returned the opposite of what its name suggests, you could have just flipped the negation on line 1469 😅
!any(!done) == all(done) // What we had
any(!done) == !all(done) // What we want
Someone likely just confused themselves with all these negations when writing this initially.
There was a problem hiding this comment.
The any vs all behavior on empty is tricky. Maybe this whole methods should have been are_all_recoveries_done indeed. I kind of wrote it just to show it's now possible to have APIs like that (including subscribing to updates), and the code is not used anywhere which means that 99% it doesn't work. :D
@maan2003 As you are working on integrating this in Fedi app, don't be afraid to completely change anything there. IMO, the most useful part of TDD is that only when you actually write the user code, you have a good feel for what good API should look like. And when you don't, you get buggy code and bad APIs. (like I did here) :D
earlier it returned true if client_recovery_progress_receiver returned empty btreemap