Clean improvements#3535
Conversation
mbaruh
added
a: moderation
| self._running_tasks = set() | ||
|
|
||
| def submit[T](self, coro: Awaitable[T]) -> asyncio.Task[T]: | ||
| """Wraps the coroutine with _semaphore logic, schedules it on the event loop, and ensures cleanup.""" |
There was a problem hiding this comment.
Nit: Use imperative mood for docs "Wrap the coroutine" rather than "Wraps the coroutine".
Also, _semaphore is an internal detail so it shouldn't be directly referenced in the docstring.
There was a problem hiding this comment.
hah the underscore is because of a refactor, but yeah don't have to mention how it's implemented.
|
|
||
| async def gather(self, return_exceptions: bool = False) -> list[Any]: | ||
| """Waits for all submitted coroutines to finish execution.""" | ||
| if self._running_tasks: |
There was a problem hiding this comment.
Nit: I think this check is redundant
There was a problem hiding this comment.
oh I thought an empty asyncio.gather raised an error, I guess not
| await self._event.wait() | ||
|
|
||
|
|
||
| class AsyncExecutor: |
There was a problem hiding this comment.
Should there be a public interface for cancelling the tasks? Might be needed if the bot is shutting down.
| """Return the datetime of the earliest message cached, or now if the cache is empty.""" | ||
| return self.bot.cached_messages[0].created_at if self.bot.cached_messages else arrow.utcnow().datetime | ||
|
|
||
| def _use_cache(self, most_recent_limit: datetime | None) -> bool: |
There was a problem hiding this comment.
Nit: Inconsistent terminology in this file. There's earliest/latest, lower/upper, first/second, and least/most recent.
| continue | ||
|
|
||
| messages_to_delete.append(message) | ||
| message_ids.append(message.id) |
There was a problem hiding this comment.
Nit: message IDs can be derived from messages_to_delete. I think it'd be cleaner to not maintain parallel lists, and that it wouldn't be a significant change in performance.
| self._running_tasks.clear() | ||
| return result | ||
|
|
||
| def cancel_all(self) -> None: |
There was a problem hiding this comment.
Should we wait for the tasks to be cancelled or is it fine to fire and forget?
|
|
||
| def cog_unload(self) -> None: | ||
| """Stop any ongoing clean.""" | ||
| self.cleaning = False |
There was a problem hiding this comment.
Why not cancel the tasks in here? Is setting cleaning to False guaranteed to cancel the tasks if any exist?
| if not self.cleaning: | ||
| return None | ||
| if old_messages: | ||
| log.trace("Some of the found messages are older than 14d, and will deleted individually.") |
There was a problem hiding this comment.
Typo: "will deleted" -> "will be deleted"
2 participants
No description provided.