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: create basic functionality for Extensions #428

Merged
merged 26 commits into from
Feb 2, 2022
Merged

feat: create basic functionality for Extensions #428

merged 26 commits into from
Feb 2, 2022

Conversation

Catalyst4222
Copy link
Contributor

@Catalyst4222 Catalyst4222 commented Jan 13, 2022

Expands Extension.new and adds new functions to mimic client decorators

About

This pr adds in basic functionality for the Extension class, along with decorators for commands/components/listeners
Marked as a draft because some things are missing, like properly removing commands on removal of the Extension
Any suggestions on improvements are welcome

Checklist

  • I've ran pre-commit to format and lint the change(s) made.
  • I've checked to make sure the change(s) work on 3.8.6 and higher.
  • This fixes/solves an Issue.
  • I've made this pull request for/as: (check all that apply)
    • Documentation
    • Breaking change
    • New feature/enhancement
    • Bugfix

@Catalyst4222
Copy link
Contributor Author

Catalyst4222 commented Jan 14, 2022

I would like someone to double check that everything works, and I'm not just hallucinating a lack of errors. It would probably be a good idea to simplify some of the code, I got it working but it doesn't look the nicest

@i0bs
Copy link
Contributor

i0bs commented Jan 14, 2022

Delta and I will test this code when this PR is marked ready for reviewing

@Catalyst4222 Catalyst4222 marked this pull request as ready for review January 14, 2022 21:54
@i0bs i0bs assigned i0bs and FayeDel Jan 14, 2022
@i0bs i0bs added breaking This contains breaking changes documentation Improvements or additions to documentation enhancement New feature or request priority This Issue/PR must be resolved first before accepting any others pending Pending approve labels Jan 14, 2022
@i0bs i0bs requested review from i0bs and FayeDel January 14, 2022 22:59
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.pyi Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
@FayeDel
Copy link
Collaborator

FayeDel commented Jan 15, 2022

There's support for @client.component, @Client.command, etc using these, but what about @Client.autocomplete?

@i0bs
Copy link
Contributor

i0bs commented Jan 15, 2022

No support for modals either

Copy link
Contributor

@i0bs i0bs left a comment

Choose a reason for hiding this comment

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

LGTM. Please pre-commit though.

interactions/client.py Outdated Show resolved Hide resolved
Co-authored-by: Max Hirtz <maxyolo01.ytb@gmail.com>
@i0bs
Copy link
Contributor

i0bs commented Jan 29, 2022

This is LGTM, I think we're finally ready to go ahead and merge this. I'll be leaving the last approval on this for @DeltaXWizard , but all of these changes look good on my end.

@FayeDel
Copy link
Collaborator

FayeDel commented Jan 29, 2022

You may need to run pre-commit again after new pending changes, I'll test afterwards though :)
(Code looks good, I have no objections, I just need to test the thing before I approve)

@mAxYoLo01
Copy link
Contributor

mAxYoLo01 commented Jan 29, 2022

After some more tests, I figured out that there still are a few problems to address:

  1. When loading any extension after bot.start(), all commands that are initialized outside of a cog (for example initialized in the main launch file) will all disappear from the Slash Commands Menu. Commands of the loaded cog will not be removed from the Menu but will not respond either.
  2. After unloading an extension, the commands from this extension are not removed, and can still be accessed from the Slash Commands Menu.

Feel free to ask me if you need some example code to recreate these issues.

@Toricane
Copy link
Contributor

Toricane commented Jan 29, 2022

I don't think you are supposed to load extensions after bot.start()
But it does seem like an issue, shouldn't be wiping the ones in the main file

@Catalyst4222
Copy link
Contributor Author

I want to test again, just to make sure I fixed the breaking changes from the last patch

@mAxYoLo01
Copy link
Contributor

mAxYoLo01 commented Jan 29, 2022

I don't think you are supposed to load extensions after bot.start() But it does seem like an issue, shouldn't be wiping the ones in the main file

Didn't know it might be a possibility that it shouldn't be loaded afterwards, in fact I was creating a Slash Command that allows the user to un/re/load cogs directly via Discord. I've previously done it with d.py, so I thought that it would be a feature that i.py should also have.

@EdVraz
Copy link
Contributor

EdVraz commented Jan 29, 2022

Afaik fl0w wanted something like hot reload for cogs at first so it should be possible tho shouldn't it?

@Toricane
Copy link
Contributor

Afaik fl0w wanted something like hot reload for cogs at first so it should be possible tho shouldn't it?

Oh ok never mind then

interactions/client.py Show resolved Hide resolved
@Catalyst4222
Copy link
Contributor Author

I don't know how to do pre-commit
I do git add <changes>, black.exe ., isort.exe ., flake8.exe . and do the changes, but it still failes

@Catalyst4222
Copy link
Contributor Author

image

@mAxYoLo01
Copy link
Contributor

Alright there is still the problem that "global" commands (not initialized in cogs) disappear after loading any cog on my side. The other issues have been fixed though.

@Catalyst4222
Copy link
Contributor Author

"global" commands (not initialized in cogs) disappear after loading any cog on my side

I'm unable to reproduce this. All commands outside of the cog remain usable, and any inside get added/removed as the cog gets loaded/unloaded. The only reason I can think of for it happening would be global commands, as I've only been testing in the guild scope. I'll try global commands now, but I don't expect a difference

@mAxYoLo01
Copy link
Contributor

The only reason I can think of for it happening would be global commands, as I've only been testing in the guild scope.

Oh my bad on this one, I formulated badly my previous message. In fact, I also have tried it only with a guild scope.
Weird that we don't have the same behaviour, on my hand whenever I load/reload a cog, all the commands outside of this cog disappear.

@Catalyst4222
Copy link
Contributor Author

I've recreated the missing commands issue, and it seems to only happen when a cog is unloaded then reloaded in quick succession. Any ideas as to why or where this is happening is welcome

@i0bs
Copy link
Contributor

i0bs commented Feb 2, 2022

I'll be moving through with merging this to unstable, I'm looking to hit a feature lock on getting extensions in rather soon and we can always make revisions with the pull request being brought in. For any relating concerns to this, I advise others to create an Issue or make a mention of it in the #unstable-dev channel in our support server.

@i0bs i0bs merged commit 5dc76a0 into interactions-py:unstable Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This contains breaking changes documentation Improvements or additions to documentation enhancement New feature or request pending Pending approve priority This Issue/PR must be resolved first before accepting any others
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants