Skip to content

Bump Discord.py to latest alpha and use BotBase from bot-core#2118

Merged
ChrisLovering merged 43 commits into
mainfrom
bump-d.py-version
Apr 20, 2022
Merged

Bump Discord.py to latest alpha and use BotBase from bot-core#2118
ChrisLovering merged 43 commits into
mainfrom
bump-d.py-version

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering commented Mar 19, 2022

This is quite a large PR due to the breaking changes from Discord.py. Most notably there was a large change around asyncio, which you can read here.

With cogs being changed to have async cog_load (and unload) functions, I have updated all the places that previously used the init_task pattern to instead now use this async cog_load function.

This also impacted how the bot is initialised and loaded, since you cannot use wait_for or similar within these async functions. As such, we created BotBase which abstracts this, and many other things, away from you.

These drastic changes also meant quite a few changes to the tests needed to be made.

I have tried to keep the commits fairly atomic, so review commit-by-commit may be best.

@ChrisLovering ChrisLovering force-pushed the bump-d.py-version branch 3 times, most recently from 7532128 to c5ab091 Compare March 19, 2022 18:09
@MarkKoz MarkKoz self-assigned this Mar 20, 2022
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

Not an extensive review but it's a start.

Comment thread bot/bot.py Outdated
Comment thread botcore/bot.py Outdated
Comment thread bot/bot.py Outdated
Comment thread botcore/bot.py Outdated
Comment thread bot/bot.py Outdated
@Xithrius Xithrius added a: dependencies Related to package dependencies and management s: WIP Work In Progress a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) t: enhancement Changes or improvements to existing features p: 2 - normal Normal Priority labels Mar 23, 2022
@ChrisLovering ChrisLovering force-pushed the bump-d.py-version branch 3 times, most recently from 3853986 to b1b3569 Compare March 31, 2022 20:55
@ChrisLovering ChrisLovering changed the title Bump Discord.py to latest alpha Bump Discord.py to latest alpha and use BotBase from bot-core Apr 1, 2022
@ChrisLovering ChrisLovering force-pushed the bump-d.py-version branch 2 times, most recently from e7ecabb to 501af18 Compare April 2, 2022 21:58
@ChrisLovering ChrisLovering marked this pull request as ready for review April 2, 2022 21:58
@ChrisLovering ChrisLovering added s: needs review Author is waiting for someone to review and approve and removed s: WIP Work In Progress labels Apr 2, 2022
@ChrisLovering ChrisLovering requested a review from MarkKoz April 3, 2022 17:21
@ChrisLovering
Copy link
Copy Markdown
Member Author

Rebased onto main. Made resources cog use async setup too, since it was added after this PR was opened.

@ChrisLovering ChrisLovering added p: 1 - high High Priority and removed p: 2 - normal Normal Priority labels Apr 16, 2022
Comment thread bot/exts/moderation/modpings.py
@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Apr 18, 2022

Hi Chris,

Thank you for your tremendous efforts in getting the bot up to date. I am sure the rest of the team is filled with excitement at the prospect of using all the new features and maintaining a slimmer code base thanks to your work on bot-core.

I went through every commit and every line. Looks like everything is order. Notwithstanding, there are many changes, and it is easy for my eyes to miss something. Unfortunately, I did not run any of the code, nor do I really have time to do so and extensively test any of it. Would you still like me to leave an approval?

@ChrisLovering
Copy link
Copy Markdown
Member Author

Hi Chris,

Thank you for your tremendous efforts in getting the bot up to date. I am sure the rest of the team is filled with excitement at the prospect of using all the new features and maintaining a slimmer code base thanks to your work on bot-core.

I went through every commit and every line. Looks like everything is order. Notwithstanding, there are many changes, and it is easy for my eyes to miss something. Unfortunately, I did not run any of the code, nor do I really have time to do so and extensively test any of it. Would you still like me to leave an approval?

If you're happy with the code quality, I'd appreciate the approval. I think as for regression testing (since there should be a lot) I'll post something in the server and see if we can crowd source it.

Comment thread bot/__main__.py Outdated
@python-discord-policy-bot python-discord-policy-bot Bot requested a review from a team April 19, 2022 09:38
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Great PR! Just some small things. I haven't tested this, but noticed it was being tested on the test server so should be fine. Might try and play around with it if I get a chance.

Comment thread pyproject.toml Outdated
Comment thread bot/exts/recruitment/talentpool/_review.py Outdated
Comment thread bot/exts/help_channels/_cog.py Outdated
Comment thread bot/exts/help_channels/_cog.py Outdated
Comment thread tests/bot/exts/info/test_help.py Outdated
Comment thread bot/exts/fun/off_topic_names.py Outdated
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Changes all look good - managed to give it a quick test run and everything I tried worked nicely! Just one thing I noticed.

Comment thread bot/exts/help_channels/_message.py Outdated
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ChrisLovering ChrisLovering merged commit 18c3bdf into main Apr 20, 2022
@ChrisLovering ChrisLovering deleted the bump-d.py-version branch April 20, 2022 21:13
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: dependencies Related to package dependencies and management p: 1 - high High Priority t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants