feat: add run_in_parallel method and tests#256
feat: add run_in_parallel method and tests#256LucasAlvws wants to merge 17 commits intoautoscrape-labs:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pydoll/browser/chromium/base.py
Outdated
| if self.options.max_parallel_tasks and self._semaphore is None: | ||
| if self._semaphore_lock is None: | ||
| self._semaphore_lock = asyncio.Lock() | ||
|
|
||
| async with self._semaphore_lock: | ||
| # Double-check pattern to avoid creating multiple semaphores | ||
| if self._semaphore is None: | ||
| self._semaphore = asyncio.Semaphore(self.options.max_parallel_tasks) |
There was a problem hiding this comment.
A property would be better for this logic:
@property
def _semaphore(self) -> asyncio.Semaphore | None:
if not self.options.max_parallel_tasks:
return None
# this is lazy
if self._sem_instance is None:
self._sem_instance = asyncio.Semaphore(self.options.max_parallel_tasks)
return self._sem_instanceThere was a problem hiding this comment.
a cached property is also really great for this case:
@cached_property
def _semaphore(self) -> asyncio.Semaphore | None:
if not self.options.max_parallel_tasks:
return None
return asyncio.Semaphore(self.options.max_parallel_tasks)
pydoll/browser/chromium/base.py
Outdated
| if self._semaphore_lock is None: | ||
| self._semaphore_lock = asyncio.Lock() | ||
|
|
||
| async with self._semaphore_lock: |
There was a problem hiding this comment.
this lock is unecessary since the creation of the semaphore is sync. The GIL will take care of it, so you can remove all the logic for the lock
pydoll/browser/chromium/base.py
Outdated
| async def run_gather(): | ||
| return await asyncio.gather(*wrapped, return_exceptions=False) | ||
|
|
||
| # Check if we're in the same event loop |
There was a problem hiding this comment.
you can remove most of the comments, try to keep only the really necessary ones.
we can add more information about this in the documentation
pydoll/browser/chromium/base.py
Outdated
| logger.info(f'Resetting permissions (context={browser_context_id})') | ||
| return await self._execute_command(BrowserCommands.reset_permissions(browser_context_id)) | ||
|
|
||
| async def run_in_parallel(self, *coroutines: Coroutine[Any, Any, Any]) -> list[Any]: |
There was a problem hiding this comment.
for the typing, a generic would be great:
T = TypeVar("T")
async def run_in_parallel(self, *coroutines: Coroutine[Any, Any, T]) -> list[T]: ...that way, the return type of the coroutines will be reflected directly in the return type of the method:
async def get_number() -> int: ...
async def get_text() -> str: ...
# here, T will be resolved as Union[int, str]
results = await browser.run_in_parallel(get_number(), get_text())
# results will be resolved to list[str | int]There was a problem hiding this comment.
so i can just add T = TypeVar("T") in the TYPE_CHECKING nested imports?
There was a problem hiding this comment.
or should i add it just before the class Browser like the logger = logging.getLogger(name)
There was a problem hiding this comment.
I'm not sure, but i think it can be under the if TYPE_CHECKING, just remember to import annotations:
from future import __annotations__3ec7f41 to
a50f318
Compare
pydoll/browser/chromium/base.py
Outdated
| return ws | ||
|
|
||
| @cached_property | ||
| def _semaphore(self) -> asyncio.Semaphore | None: |
There was a problem hiding this comment.
I usually like to put the properties in the top of the class, can you do that?
thalissonvs
left a comment
There was a problem hiding this comment.
some logs would also be great, try to keep most of them as debug
d2c8b63 to
0ba0fa7
Compare
Co-authored-by: Thalison Fernandes <thalisondev@gmail.com>
Pull Request
Description
Related Issue(s)
Type of Change
How Has This Been Tested?
# Include code examples if relevantTesting Checklist
Screenshots
Implementation Details
API Changes
Additional Info
Checklist before requesting a review
poetry run task lintand fixed any issuespoetry run task testand all tests pass