Issue816 threadeddownload #836
Conversation
soxofaan
left a comment
soxofaan
left a comment
There was a problem hiding this comment.
some more quick notes I managed to squeeze in
| """ | ||
|
|
||
| def __init__(self, max_workers: int = 2): | ||
| def __init__(self, max_workers: int = 1, name: str = 'default'): |
There was a problem hiding this comment.
It's not necessary to give a a name here right? or how would that be used?
I think there is something wrong with this feature branch or pull request (rebase gone wrong?) |
|
oh that is odd, will indeed cherrypick and redo the rebase |
HansVRP
force-pushed
the
issue816_threadeddownload
branch
from
3234f4f to
597997f
Compare
…rs; this can be used to guarantee the download gets finished
…'; similarly download shutdown depend on optional download
HansVRP
force-pushed
the
issue816_threadeddownload
branch
from
597997f to
c999d1f
Compare
|
@soxofaan apologies for not noticing myself, branch is back in order |
|
good to merge @soxofaan ? |
| def __init__(self, max_workers: int = 1, name: str = 'default'): | ||
| self._executor = concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) | ||
| self._future_task_pairs: List[Tuple[concurrent.futures.Future, Task]] = [] | ||
| self._name = name |
There was a problem hiding this comment.
What is the purpose of this name? I don't see it being used somewhere
| :param max_workers: | ||
| Maximum number of concurrent threads to use for execution. | ||
| Defaults to 2. | ||
| Defaults to 1. |
There was a problem hiding this comment.
Is there a reason to change this default?
|
|
||
| self._future_task_pairs = to_keep | ||
| return results, len(to_keep) | ||
| return results |
There was a problem hiding this comment.
Is there an important reason to remove len(to_keep) here?
Also note that return type annotation and documentation are not updated yet here
|
|
||
| def number_pending_tasks(self) -> int: | ||
| """Return the number of tasks that are still pending (not completed).""" | ||
| return len(self._future_task_pairs) |
There was a problem hiding this comment.
Note that separating number_pending_tasks from process_futures might be risky/misleading as number_pending_tasks only makes sense if process_futures has run recently enough, otherwise it will return possibly outdated information.
| results = pool.process_futures(timeout) | ||
| all_results.extend(results) | ||
|
|
||
| return all_results |
There was a problem hiding this comment.
return type does not correspond with type annotation here
No description provided.