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.
| """Helper function for getting messages from the cache.""" | ||
| message_mappings = defaultdict(list) | ||
| message_ids = [] | ||
| message_mappings = {channel: [] for channel in channels} |
There was a problem hiding this comment.
Why not leave it as a defaultdict?
| self, | ||
| channels: Iterable[TextChannel], | ||
| executor: AsyncExecutor, | ||
| *, |
There was a problem hiding this comment.
Nit: why make them keyword-only?
There was a problem hiding this comment.
I don't like having this many distinct positional arguments, and the function was being called with kw already anyway
| 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.
| if self.is_older_than_14d(message): | ||
| # Further messages are too old to be deleted in bulk | ||
| delete_old = True | ||
| old_messages[channel] = messages[current_index:] |
There was a problem hiding this comment.
Should add to the docstring that this function expects messages to be sorted by timestamp.
| # Means that the cleaning was canceled | ||
| return None | ||
| if old_messages: | ||
| old_deleted = await self._delete_messages_individually(old_messages) |
There was a problem hiding this comment.
Why do old messages need to be deleted individually?
2 participants
No description provided.