Skip to content

Disnake migration#2103

Merged
ChrisLovering merged 4 commits into
mainfrom
disnake-migration
Mar 6, 2022
Merged

Disnake migration#2103
ChrisLovering merged 4 commits into
mainfrom
disnake-migration

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

@ChrisLovering ChrisLovering commented Feb 25, 2022

This migrates the bot to disnake, as per recent discussions.

What this includes:

  • Changes all references from discord.py to disnake, including in tests
  • Bump version of bot-core so discord.py wasn't required as a sub dep
  • Use monkey patches from bot-core
  • Fix the single breaking change I could find
    • The internal bot.http.edit_message function now has a required files kwarg, which is only used in the help channels cog.
    • Set this to None as we never upload files using that message

This not included in this PR:

  • Updating the tags to reference disnake. These were left with the discord module, and can be updated later if needed
  • Migrating the bot to use all utils that have been moved to bot-core
    • This would inflate this PR even further, so didn't want to pull it into scope.

P.S this is quite a large PR, so reviewing by the commits may be helpful. I tried to keep the huge rename within the single commit 27b18ba, and didn't include anything else in there.

@ChrisLovering ChrisLovering added a: dependencies Related to package dependencies and management a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: API Related to or causes API changes p: 1 - high High Priority a: tests Related to tests (e.g. unit tests) t: enhancement Changes or improvements to existing features s: needs review Author is waiting for someone to review and approve labels Feb 25, 2022
Comment thread bot/__init__.py Outdated
commands.command = partial(commands.command, cls=monkey_patches.Command)
commands.GroupMixin.command = partialmethod(commands.GroupMixin.command, cls=monkey_patches.Command)
# Apply all monkey patches from bot core.
monkey_patches.apply_monkey_patches()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This API is kind of silly. If there's only going to be one public function, it would have been better to expose it directly from the utils namespace. You stated yourself that we'd probably never want to apply patches on an individual basis; if patches are no longer needed we'd just remove them entirely from the code base.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MarkKoz I think you should open an issue in the bot-core repository then. I agree with you and would like to see this changed, but at the moment this is not something Chris can change in this PR other than doing from botcore.utils.monkey_patches import apply_monkey_patches.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Raised python-discord/bot-core#35 for this comment, good point Mark

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 9e6989a

Comment thread pyproject.toml Outdated
@Xithrius Xithrius requested review from Bluenix2, HassanAbouelela, bast0006, fisher60 and kosayoda and removed request for Den4200 and ks129 February 25, 2022 04:44
@Bluenix2
Copy link
Copy Markdown
Contributor

Just as point of order / reminder, please do not force push in this PR as that will mess with the "Viewed" markers to the right.

@Bluenix2
Copy link
Copy Markdown
Contributor

Good work on this by the way, I ran both Pyright and Mypy in the command line with most type checking disabled (or ignored when reading the output) and I could not find any complaints about discord or missing imports (although Mypy failed to find the typings for disnake.ext.commands which may make this test incomplete).

Copy link
Copy Markdown
Contributor

@Bluenix2 Bluenix2 left a comment

Choose a reason for hiding this comment

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

I found these three references to discord.py still, they should probably be renamed as well:

as this can cause incorrect objects being used by discordpy's converters.

For forwardrefs in command annotations discordpy uses the __global__ attribute of the function

"""Update the decorated function to look like `wrapped` and update globals for discordpy forwardref evaluation."""

@Bluenix2
Copy link
Copy Markdown
Contributor

Bluenix2 commented Mar 3, 2022

Thanks for addressing my comments Chris. I have looked at the diff of each file and not yet found any odd changes - I could also not find any differences between discord.py and disnake when migrating other than the new text-in-voice feature resulting in some methods returning VoiceChannel which may not include all attributes/methods (send() and edit() would be present I believe). Any issues that may arise from this that come to mind?

Additionally, this pull request is pretty much incompatible with every other pull request currently open. Has there been any thought put into figuring out which ones should be merged before this is merged, and which ones should be merged afterwards (so that we can let the author of those know to update to disnake)?

@ChrisLovering
Copy link
Copy Markdown
Member Author

ChrisLovering commented Mar 3, 2022

[ ... ] VoiceChannel** which may not include all attributes/methods (send() and edit() would be present I believe). Any issues that may arise from this that come to mind?

Not that I can think of, we also don't use text in voice channels in the server.

Additionally, this pull request is pretty much incompatible with every other pull request currently open. Has there been any thought put into figuring out which ones should be merged before this is merged, and which ones should be merged afterwards (so that we can let the author of those know to update to disnake)?

I think any PRs that are open when this is merged will just need to be rebased, I don't think there are any PRs open that should block this.

We of course can extend the offer to help anyone with an open PR to resolve conflicts.

@ChrisLovering ChrisLovering added the review: do not merge The PR can be reviewed but cannot be merged now label Mar 3, 2022
@ChrisLovering
Copy link
Copy Markdown
Member Author

Marked as do not merge to allow me to squash fixup commits on this PR before it hits main

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.

I've looked over all the diffs; looks fine except that one comment. I ran the bot and it started up fine. I did my own search for missed references and couldn't find anything.

Comment thread bot/utils/checks.py Outdated
Copy link
Copy Markdown
Member

@mbaruh mbaruh left a comment

Choose a reason for hiding this comment

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

Ran most commands, seems to work well. Thanks!

@ChrisLovering ChrisLovering removed the review: do not merge The PR can be reviewed but cannot be merged now label Mar 5, 2022
@ChrisLovering ChrisLovering merged commit acaae30 into main Mar 6, 2022
@ChrisLovering ChrisLovering deleted the disnake-migration branch March 6, 2022 00:00
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: API Related to or causes API changes a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: dependencies Related to package dependencies and management a: tests Related to tests (e.g. unit tests) 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.

5 participants