Skip to content
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

feat: asynchronousifying setup function in cogs #536

Merged
merged 8 commits into from Mar 25, 2022
Merged

feat: asynchronousifying setup function in cogs #536

merged 8 commits into from Mar 25, 2022

Conversation

MaskDuck
Copy link
Contributor

@MaskDuck MaskDuck commented Mar 17, 2022

Summary

NOTE: THIS WILL RAISE AN KEYBOARDINTERRUPT IF YOU ARE DOING AS SKELMIS SAYS AND IS ENDING THE BOT USING CONTROL + C

setup to be awaitable, fix issue #468

Checklist

  • If code changes were made then they have been tested. (pls help)
    • I have updated the documentation to reflect the changes. (pls help)
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed) (uh not really, but yeah)
  • This PR is not a code change (e.g. documentation, README, ...)

@MaskDuck MaskDuck requested a review from TAG-Epic as a code owner March 17, 2022 04:36
@netlify
Copy link

netlify bot commented Mar 17, 2022

Deploy Preview for nextcord-gh-action ready!

Name Link
🔨 Latest commit 82be889
🔍 Latest deploy log https://app.netlify.com/sites/nextcord-gh-action/deploys/623c567a31cecd00087c763e
😎 Deploy Preview https://deploy-preview-536--nextcord-gh-action.netlify.app/faq
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@MaskDuck

This comment has been minimized.

@MaskDuck

This comment has been minimized.

Copy link
Collaborator

@Skelmis Skelmis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make your pull request name better, ideally following cc.
Something like feat: Allow cog setup to be async

nextcord/ext/commands/bot.py Outdated Show resolved Hide resolved
@Skelmis
Copy link
Collaborator

Skelmis commented Mar 17, 2022

I personally disagree with making this a breaking change, especially given the overall use-case from what I have seen. Yes, some form of async loading is nice for a minority, however, enforcing the syntax proposed essentially removes .run which is what the majority of bots uses.

@MaskDuck

This comment has been minimized.

@Soheab
Copy link
Contributor

Soheab commented Mar 17, 2022

You could use nextcord.utils.maybe_coroutine() for this.

@Skelmis
Copy link
Collaborator

Skelmis commented Mar 19, 2022

You could use nextcord.utils.maybe_coroutine() for this.

This requires making load_ext.. async, which I have already commented on.

Copy link
Member

@ooliver1 ooliver1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make an faq entry on the error that may be risen as discussed

@ooliver1 ooliver1 added t: enhancement Type: enhancement - new feature or request s: in progress Status: the issue or PR is in development/progress p: low Priority: low - not important to be worked on labels Mar 21, 2022
@MaskDuck

This comment has been minimized.

@DenverCoder1
Copy link
Collaborator

DenverCoder1 commented Mar 23, 2022

In extensions.rst this should be changed. You can remove "This setup must be a plain Python function (not a coroutine)."

An extension at its core is a python file with an entry point called setup. This setup must be a plain Python function (not a coroutine). It takes a single parameter -- the :class:~.commands.Bot that loads the extension.

@ooliver1
Copy link
Member

also perhaps handle error then raise an error which shows the faq entry perhaps

@MaskDuck

This comment has been minimized.

@MaskDuck MaskDuck changed the title uh setup now can being awaitable =) 🦶: asynchronousifying setup function in cogs Mar 24, 2022
@MaskDuck MaskDuck changed the title 🦶: asynchronousifying setup function in cogs feat: asynchronousifying setup function in cogs Mar 24, 2022
@MaskDuck

This comment has been minimized.

@Skelmis
Copy link
Collaborator

Skelmis commented Mar 24, 2022

guys i have tested the code

. . . . .

Have you also tested backwards compatibility?

@MaskDuck

This comment has been minimized.

docs/faq.rst Outdated Show resolved Hide resolved
nextcord/ext/commands/bot.py Outdated Show resolved Hide resolved
@MaskDuck

This comment has been minimized.

@Skelmis
Copy link
Collaborator

Skelmis commented Mar 24, 2022

(if you mean plain python func setup then i have also tested)

Sounds good

@MaskDuck

This comment has been minimized.

@Skelmis
Copy link
Collaborator

Skelmis commented Mar 24, 2022

TIL asyncio.run is indeed not blocking. So ty for that

However, still not removing the requested changes however as it is bad practice since it's creating two loops (I think) and either of the two calls should be the entry point for the bot.

async def main():
    bot.load_extension("cog_name")
    await bot.start("TOKEN")


asyncio.run(main())

Tested on your fork works fine and as expected

@MaskDuck

This comment has been minimized.

@MaskDuck

This comment has been minimized.

@Skelmis
Copy link
Collaborator

Skelmis commented Mar 24, 2022

Code?

Alternately you can create a task instead, and then use bot.run

Copy link
Collaborator

@Skelmis Skelmis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and I've tested on:

  • 3.8.5
  • 3.9.6
  • 3.10.0

@ooliver1 ooliver1 merged commit 27c0ded into nextcord:master Mar 25, 2022
ooliver1 pushed a commit that referenced this pull request Apr 9, 2022
* uh setup now can being awaitable =)

* add faq entry

* remove coroutine notice

* make a more useful error :trollface:

* code docs

* fix weirdo

* fix docs

* .
@EmreTech EmreTech removed the s: in progress Status: the issue or PR is in development/progress label Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: low Priority: low - not important to be worked on t: enhancement Type: enhancement - new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants