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: Allow empty iterables as a command_prefix #7809

Merged
merged 5 commits into from
Mar 31, 2022

Conversation

DenverCoder1
Copy link
Contributor

@DenverCoder1 DenverCoder1 commented Mar 30, 2022

Problem

If you don't want any prefix or even pings to trigger commands (for example if you only have slash commands or have no prefixed commands), you currently need a hackish workaround such as:

bot = commands.Bot(command_prefix=" ")  # initial whitespace is ignored in messages

Solution

This change allows passing an empty iterable to not listen for any prefix. This way developers do not need to specify a prefix if they don't have any commands that use it (for example, if the bot only uses slash commands or listeners such as on_message)

bot = commands.Bot(command_prefix=[])  # creates a Bot with no prefix

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, ...)

@AbstractUmbra
Copy link
Contributor

This feels a bit overeager as a solution to this problem.
Alternative methods exist:

Use discord.Client over commands.Bot so you do not have to process incoming messages.
Use a Bot without the messages intent, meaning they will just not receive message events.
Overriding the Bot.on_message method to do nothing, just implementing pass.

There are obviously caveats to each of these proposed solutions but they will fit needs generally.

@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Mar 30, 2022

Use discord.Client over commands.Bot so you do not have to process incoming messages.

Cogs, among other things, still require commands.Bot as far as I know, so this won't work for everyone.

Use a Bot without the messages intent, meaning they will just not receive message events.

Many people will still want to listen for messages even if they don't have a command prefix.

Overriding the Bot.on_message method to do nothing, just implementing pass.

This is true, but I feel it is much less intuitive than the alternative I am suggesting. The most obvious way one may think to disable the prefix is by setting it to None or an empty list.

Also, this change also allows you to conditionally have no prefix, for example, in some servers you may want a prefix, but in others, you can have your prefix function return None.

@AbstractUmbra
Copy link
Contributor

AbstractUmbra commented Mar 30, 2022

As I said each solution fits other needs better.

Your solution here disabling the prefix does not stop each message being processed, so the work is still being "done" to try and parse each message that comes in. If you wish to fully disable message processing then it should be done in an alternate way.
If you truly wish to disable message command processing, then it should achieve that somehow, not just setting the prefix to check against as None (or an empty list as you have).

Editing for clarity:
A solution is that during the Bot class' default on_message method, is to just skip the process_commands stage if the command_prefix attr is None.
I still feel that having the command prefix being None is overzealous, and a kwarg on the Bot constructor makes more sense and is more verbose in it's use?

@DenverCoder1
Copy link
Contributor Author

I agree that for performance, on_message can skip process_commands when command_prefix is None. For the purpose of conditional prefixes, None can still be treated as empty list.

I will make this performance enhancement.

@DenverCoder1
Copy link
Contributor Author

I'd like to hear what @Rapptz thinks given the points I have made.

To be clear, the main goal of this change is for:

  1. People who want to use extensions, cogs, listeners, etc, but don't use other features of the commands extension. It is a confusing experience to require them to choose a command prefix that is not used by any commands.
  2. People who want to disable commands in certain contexts using a prefix function. For example, one may want to write a function that returns the guild prefix if one is found in their database, or None if the prefix is not configured or specifically disabled for that guild.

@Rapptz
Copy link
Owner

Rapptz commented Mar 30, 2022

Hi.

I've rejected this before in the discord.py server. In my mind, the command extension will always be designed for regular commands first and foremost. I have no plan to specialise it for app command only usage. There are ways to get around this if you know what to do, for example overriding on_message to just not call process_commands.

Even in the post-message content intent world, commands with a mention prefix will work just fine.

@Rapptz Rapptz closed this Mar 30, 2022
@DenverCoder1
Copy link
Contributor Author

Is there any reason not to make doing that easier for the user?

This doesn't have any negative side-effects to users who do want to use commands.

Also, if the commands module is specifically for regular commands, are there any plans to separate extensions, cogs, and listeners from the module, so people can make use of them without bringing in everything else from the commands extension?

@Rapptz
Copy link
Owner

Rapptz commented Mar 30, 2022

Is there any reason not to make doing that easier for the user?

I think, at best, allowing you to pass [] should be okay. In a situation where you fetch from a database and there are no prefixes configured for some reason. Though I still think defaulting to a mention prefix is better for that use case.

Also, if the commands module is specifically for regular commands, are there any plans to separate extensions, cogs, and listeners from the module, so people can make use of them without bringing in everything else from the commands extension?

Nope.

@DenverCoder1
Copy link
Contributor Author

So you would be fine with the PR if it just allowed empty iterables, but without making the prefix optional?

@Rapptz
Copy link
Owner

Rapptz commented Mar 30, 2022

Yeah that'd be fair.

@Rapptz Rapptz reopened this Mar 30, 2022
@DenverCoder1 DenverCoder1 changed the title feat: Default command_prefix in commands.Bot feat: Allow empty iterables as a command_prefix Mar 30, 2022
@DenverCoder1
Copy link
Contributor Author

Ok, I've updated the PR.

Do you want the change to on_message to be kept (as it gives a small performance improvement when an empty list is passed)?

@Rapptz
Copy link
Owner

Rapptz commented Mar 30, 2022

There is no need to modify on_message.

@DenverCoder1
Copy link
Contributor Author

I've reverted that as well now

@Rapptz
Copy link
Owner

Rapptz commented Mar 31, 2022

Thanks.

@Rapptz Rapptz merged commit caac97c into Rapptz:master Mar 31, 2022
@DenverCoder1 DenverCoder1 deleted the default-command-prefix branch March 31, 2022 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants