Add a progress callback to the wait operation function #204
Conversation
saimanikant
requested review from
nezgrath,
ojkoenig and
FedericoNegri
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
- Coverage 84.99% 84.33% -0.67%
==========================================
Files 19 19
Lines 1193 1200 +7
==========================================
- Hits 1014 1012 -2
- Misses 179 188 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -281,6 +282,7 @@ def wait_for( | |||
| interval: float = 0.1, | |||
| cap: float = 2.0, | |||
| raise_on_error: bool = False, | |||
| progress_handler: Callable[[int], None] = None, | |||
There was a problem hiding this comment.
- The operation progress is a
float(between 0 and 1). Why the callback signature says it's anint? - The function can wait for a list of operations. How can a client know to which operation the progress applies to? Maybe we could add the operation id (string) an additional argument to the callback.
| if progress_handler is not None: | ||
| for op in ops: | ||
| progress_handler(op.progress) |
There was a problem hiding this comment.
There's already a loop over the operations few lines above. Just embed this condition in there.
tests/large_file_test.py
Outdated
| def handler(current_progress): | ||
| log.info(f"{current_progress * 100.0}% completed") |
There was a problem hiding this comment.
If you defined a callback that stores the progress data, you can then assert on it. For instance to verify that the callback was called at least say twice, that at the end progress is 1, etc.
Also, please add a test that verifies the case of multiple operations.
| @@ -296,17 +314,19 @@ def wait_for( | |||
| ops = self._operations(operation_ids) | |||
| so_far = hf.format_timespan(time.time() - start) | |||
| log.debug(f"Waiting for {len(operation_ids)} operations to complete, {so_far} so far") | |||
| if self.client.binary_config.debug: | |||
| if progress_handler is not None: | |||
There was a problem hiding this comment.
The conditions on self.client.binary_config.debug and progress_handler are not related. We should be able to print debug logs even if there's no progress_handler. I'd just do the loop twice.
Pull Request Template
Description
Addressing #86
Added tests.
Checklist
Please complete the following checklist before submitting your pull request: