Implement a least breaking approach to slash commands #39

Merged
Gnome-py merged 42 commits from 2.0 into 2.0 2021-09-18 23:28:11 +00:00
Gnome-py commented 2021-08-31 18:51:40 +00:00 (Migrated from github.com)

Summary

This PR implements a system on top of ext.commands for slash commands allowing for minimal breakage by constructing fake message objects on_interaction.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • 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)
  • This PR is not a code change (e.g. documentation, README, ...)
## Summary <!-- What is this pull request for? Does it fix any issues? --> This PR implements a system on top of ext.commands for slash commands allowing for minimal breakage by constructing fake message objects `on_interaction`. ## Checklist <!-- Put an x inside [ ] to check it, like so: [x] --> - [x] If code changes were made then they have been tested. - [x] I have updated the documentation to reflect the changes. - [ ] This PR fixes an issue. - [x] This PR adds something new (e.g. new method or parameters). - [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed) - [ ] This PR is **not** a code change (e.g. documentation, README, ...)
paris-ci (Migrated from github.com) reviewed 2021-08-31 20:47:07 +00:00
paris-ci (Migrated from github.com) left a comment

I need to test it further. I've noted that the PR contains a few style fixes for unrelated files, would you be able to remove them (or send them to another PR) so as not to clutter the commit view ?

Great work !

I need to test it further. I've noted that the PR contains a few style fixes for unrelated files, would you be able to remove them (or send them to another PR) so as not to clutter the commit view ? Great work !
paris-ci (Migrated from github.com) commented 2021-08-31 20:05:59 +00:00

Small nitpic, shouldn't this say client ?

        A coroutine to be called to setup the client, by default this is blank.
Small nitpic, shouldn't this say client ? ```suggestion A coroutine to be called to setup the client, by default this is blank. ```
@ -118,6 +160,35 @@ def _is_submodule(parent: str, child: str) -> bool:
return parent == child or child.startswith(parent + ".")
def _unwrap_slash_groups(
paris-ci (Migrated from github.com) commented 2021-08-31 20:09:04 +00:00

Maybe this could be made a list of guild ids, instead of just a single guild.

Maybe this could be made a list of guild ids, instead of just a single guild.
paris-ci (Migrated from github.com) commented 2021-08-31 20:21:19 +00:00

Given this might be overridden by subclasses, I'd suggest having a process_slash_commands coro called from there, as in on_message.

Given this might be overridden by subclasses, I'd suggest having a process_slash_commands coro called from there, as in on_message.
paris-ci (Migrated from github.com) commented 2021-08-31 20:25:59 +00:00

Combine the two calls in the init

Combine the two calls in the __init__
paris-ci (Migrated from github.com) commented 2021-08-31 20:26:30 +00:00

Or, even better, _FakeSlashMessage.from_interaction(interaction)

Or, even better, `_FakeSlashMessage.from_interaction(interaction)`
paris-ci (Migrated from github.com) commented 2021-08-31 20:35:32 +00:00

Why isn't this in BotBase ?

Why isn't this in BotBase ?
paris-ci (Migrated from github.com) commented 2021-08-31 20:37:16 +00:00

You should add docs for message_commands and slash_commands

You should add docs for `message_commands` and `slash_commands`
@ -281,2 +326,4 @@
If this is set, only upload this slash command to these guild IDs.
This overwrites the global ``slash_command_guilds`` parameter of :class:`.Bot`.
paris-ci (Migrated from github.com) commented 2021-08-31 20:40:15 +00:00

message_command would be more appropriate here for consistency.

`message_command` would be more appropriate here for consistency.
paris-ci (Migrated from github.com) commented 2021-08-31 20:43:25 +00:00

Should this really be done by the library ?

Should this really be done by the library ?
Gnome-py (Migrated from github.com) reviewed 2021-09-01 10:39:31 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-01 10:39:31 +00:00

if this isn't done by the lib, the interaction fails (no response in the channel), as this is the default help command I thought it appropriate.

if this isn't done by the lib, the interaction fails (no response in the channel), as this is the default help command I thought it appropriate.
Gnome-py (Migrated from github.com) reviewed 2021-09-01 10:40:53 +00:00
@ -118,6 +160,35 @@ def _is_submodule(parent: str, child: str) -> bool:
return parent == child or child.startswith(parent + ".")
def _unwrap_slash_groups(
Gnome-py (Migrated from github.com) commented 2021-09-01 10:40:53 +00:00

resolved in #bikeshedding

resolved in #bikeshedding
Gnome-py (Migrated from github.com) reviewed 2021-09-01 10:42:52 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-01 10:42:52 +00:00

Bot is used in other docstrings throughout Client, such as clear and create_guild.

Bot is used in other docstrings throughout Client, such as `clear` and `create_guild`.
isaa-ctaylor (Migrated from github.com) reviewed 2021-09-01 10:46:11 +00:00
@ -118,6 +160,35 @@ def _is_submodule(parent: str, child: str) -> bool:
return parent == child or child.startswith(parent + ".")
def _unwrap_slash_groups(
isaa-ctaylor (Migrated from github.com) commented 2021-09-01 10:46:10 +00:00

I think the idea was to discourage use of guild slash commands where global slash commands could be used, then have a kwarg in command deco for specific guilds:
image

I think the idea was to discourage use of guild slash commands where global slash commands could be used, then have a kwarg in command deco for specific guilds: ![image](https://user-images.githubusercontent.com/64303279/131658493-f385921a-a3c8-4154-b095-8fb1f8575e64.png)
Gnome-py (Migrated from github.com) reviewed 2021-09-01 10:48:33 +00:00
@ -118,6 +160,35 @@ def _is_submodule(parent: str, child: str) -> bool:
return parent == child or child.startswith(parent + ".")
def _unwrap_slash_groups(
Gnome-py (Migrated from github.com) commented 2021-09-01 10:48:33 +00:00

yes, maybe I marked resolved prematurely

yes, maybe I marked resolved prematurely
Gnome-py (Migrated from github.com) reviewed 2021-09-01 10:53:55 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-01 10:53:55 +00:00

needs access to self.application_id or (await self.application_info()) which are from discord.Client, if you can think of a better way to do it let me know

needs access to `self.application_id or (await self.application_info())` which are from discord.Client, if you can think of a better way to do it let me know
Gnome-py (Migrated from github.com) reviewed 2021-09-01 13:47:20 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-01 13:47:20 +00:00

Added, alongside docs for message_command and slash_command in the command decorators.

Added, alongside docs for `message_command` and `slash_command` in the command decorators.
Gnome-py (Migrated from github.com) reviewed 2021-09-01 16:32:36 +00:00
@ -118,6 +160,35 @@ def _is_submodule(parent: str, child: str) -> bool:
return parent == child or child.startswith(parent + ".")
def _unwrap_slash_groups(
Gnome-py (Migrated from github.com) commented 2021-09-01 16:32:36 +00:00

decided to do both

decided to do both
Daggy1234 (Migrated from github.com) requested changes 2021-09-02 17:29:56 +00:00
Daggy1234 (Migrated from github.com) left a comment

I'll help with tthe typing later, but tons of type:ignore

I'll help with tthe typing later, but tons of `type:ignore`
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:16:39 +00:00

Needs a versionadd

Needs a versionadd
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:17:49 +00:00

Nitpick, but I think avoiding # type :ignore is a good precedent

Nitpick, but I think avoiding `# type :ignore` is a good precedent
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:19:30 +00:00

better to if and raise an error, I think that comes to api design. But I think a better error for the user is necessary instead of an simple assertionerror

better to `if` and raise an error, I think that comes to api design. But I think a better error for the user is necessary instead of an simple assertionerror
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:20:21 +00:00

Docstring for function?

Docstring for function?
@ -405,0 +465,4 @@
kwargs.pop("nonce", None)
kwargs.pop("stickers", None)
kwargs.pop("reference", None)
kwargs.pop("delete_after", None)
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:21:12 +00:00

Content not in func docstring

Content not in func docstring
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:23:29 +00:00

ValueError? use an ext.command error right like the registration error

ValueError? use an ext.command error right like the registration error
Daggy1234 (Migrated from github.com) commented 2021-09-02 17:25:39 +00:00

Reads better as "Parameter 'intents' must be of :class:discord.Intent"

Reads better as "Parameter 'intents' must be of :class:`discord.Intent`"
Gnome-py (Migrated from github.com) reviewed 2021-09-02 19:28:36 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-02 19:24:29 +00:00

where would I put it then, I have to somewhere to make the fake message into a discord.Message for typing purposes

where would I put it then, I have to somewhere to make the fake message into a discord.Message for typing purposes
Gnome-py (Migrated from github.com) commented 2021-09-02 19:25:23 +00:00

this is a situation that should never happen, and the assert is only for typing purposes, None was popped from the dict above.

this is a situation that should never happen, and the assert is only for typing purposes, None was popped from the dict above.
@ -405,0 +465,4 @@
kwargs.pop("nonce", None)
kwargs.pop("stickers", None)
kwargs.pop("reference", None)
kwargs.pop("delete_after", None)
Gnome-py (Migrated from github.com) commented 2021-09-02 19:26:41 +00:00

Content is from abc.Messageable.send and

This function takes all the parameters of :meth:`.abc.Messageable.send`

Content is from `abc.Messageable.send` and > This function takes all the parameters of :meth:\`.abc.Messageable.send\`
Gnome-py (Migrated from github.com) commented 2021-09-02 19:27:18 +00:00

Not a change from this PR, weird git stuff is going on.

Not a change from this PR, weird git stuff is going on.
Gnome-py (Migrated from github.com) reviewed 2021-09-02 20:28:37 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-02 20:27:24 +00:00

Resolved in 8a779ef595

Resolved in 8a779ef595a8c167309110eeb4de28b8e0932dc6
Gnome-py (Migrated from github.com) commented 2021-09-02 20:27:41 +00:00

Resolved in 8a779ef595

Resolved in 8a779ef595a8c167309110eeb4de28b8e0932dc6
Gnome-py (Migrated from github.com) commented 2021-09-02 20:28:04 +00:00

Resolved in 8a779ef595

Resolved in 8a779ef595a8c167309110eeb4de28b8e0932dc6
IAmTomahawkx (Migrated from github.com) requested changes 2021-09-11 22:31:07 +00:00
IAmTomahawkx (Migrated from github.com) commented 2021-09-11 22:07:44 +00:00

TypeError isnt the right type of error to raise here.
Consider ValueError

TypeError isnt the right type of error to raise here. Consider ValueError
IAmTomahawkx (Migrated from github.com) commented 2021-09-11 22:10:03 +00:00

try pulling from cache first here maybe?

try pulling from cache first here maybe?
IAmTomahawkx (Migrated from github.com) commented 2021-09-11 22:22:21 +00:00

This also should be a ValueError

This also should be a ValueError
Gnome-py (Migrated from github.com) reviewed 2021-09-12 10:46:04 +00:00
Gnome-py (Migrated from github.com) commented 2021-09-12 10:46:03 +00:00

interaction.channel is a property that does a cache lookup using the ID, if it is None or PartialMessageable it couldn't get from cache

interaction.channel is a property that does a cache lookup using the ID, if it is None or PartialMessageable it couldn't get from cache
Sign in to join this conversation.
No description provided.