-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Asynchronous cog/extension loading. #1132
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: lena <77104725+elenakrittik@users.noreply.github.com>
@@ -0,0 +1 @@ | |||
|commands| :meth:`.ext.commands.Cog.cog_load` and :meth:`.ext.commands.Cog.cog_unload` can now be either asynchronous or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is this wording okay?
@loop.setter | ||
def loop(self, _value: Never) -> None: | ||
warnings.warn( | ||
"Setting `Client.loop` is deprecated and has no effect. Use `asyncio.get_running_loop()` instead.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: i'm not sure the suggestion is correct (apart from the typo (get
=> set
), i don't know if set_event_loop
will work on-the-fly); might be better to remove the suggestion altogether?
Signed-off-by: lena <77104725+elenakrittik@users.noreply.github.com>
Summary
Fixes #641
Refactors the codebase by removing loop sharing where possible and using
asyncio.get_running_loop()
instead. The loop is still shared in places where the code runs on a separate thread likeKeepAliveHandler
andAudioPlayer
. This means that the majority of the library can now assume that there is an event loop already running. Other changes are documented in changelog files.While the amount of breaking changes may seem horrific, practically most (if not all) use cases are trivial to fix: users will only need to paint a plenty of async/awaits here and there and possibly move any initialization logic from
on_ready
, bot constructors, etc. intosetup_hook
. For an example of migrating seetest_bot
changes, or for a real-world example see these three commits on my Avrae fork.Thanks to the addition of
Client.loop
property bots that currently do something likebot.loop.create_task
will not be affected by the changes (as long as that access happens after the bot is started, of course).While i have tried my best to test this in as many cases as possible, i would appreciate if someone could port their bot and see if there is anything suspicious happening.
Notably, i haven't yet tested voice sending features, even though they probably should be fine as-is.Tested it usingexamples/basic_voice.py
, works like a charm now!I would also appreciate some hints on how/where to document this change, as i believe while it is easy to fix, it is still something that we need to make as many users as possible aware of even outside of the changelog.
As for the "Port #xxxx" commits: this initially started as ports of the according PRs to discord.py, but since the 4th or so commit work was done without "stealing" code from there. Not sure if there's anything legal needed to be done?
Checklist
pdm lint
pdm pyright