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

Support app command contexts #9406

Closed
wants to merge 15 commits into from

Conversation

RedKinda
Copy link
Contributor

Summary

Adds @dm_only and @private_only decorators to allow specifying the new contexts field in app commands, as specified here, as well as the relevant init args.

The docs mention that if this field is specified, the permission calculation ignores the dm_permission field. Not sure how this should be reflected in the library for backwards compatibility purposes.

Checklist

  • If code changes were made then they have been tested. (needs a bit more testing for groups and context commands)
    • 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, ...)

discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/app_commands/models.py Outdated Show resolved Hide resolved
discord/enums.py Outdated Show resolved Hide resolved
Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Just some minor bikeshedding/comments for the first pass:

  1. AppCommandContext is not an enum, it's actually a flag type. Discord has recently been doing bitfields using an array because they falsely believe it leads to better developer experience since they are targeting developers that do not know how to bitshift. I suggest migrating this over to use the new ArrayFlags type and work with it. I have tried to get them to change this to a regular integer but they seem to want to not want to change it to the more space-efficient and sane format.
  2. allowed_contexts and any external reference to context in this well context should probably be called restrict instead. The type should still be called AppCommandContext but the attribute (and maybe eventual decorator) should be called restrict.
  3. private_only should probably be renamed to private_channel_only for consistency with the library (e.g. abc.PrivateChannel).
  4. The flag names for AppCommandContext should not use the Discord naming scheme since they are bad, instead it should probably be guild, dm_channel, and private_channel.

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

You're missing the documentation for the new decorators. They need to be added to the API reference. Other than that this PR looks good.

discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/flags.py Outdated Show resolved Hide resolved
discord/flags.py Outdated Show resolved Hide resolved
@RedKinda RedKinda marked this pull request as ready for review May 17, 2023 11:13
discord/app_commands/commands.py Outdated Show resolved Hide resolved
discord/app_commands/models.py Outdated Show resolved Hide resolved
@Rapptz Rapptz added the pending PR implements an in-progress or explicitly unreleased Discord feature label May 20, 2023
@@ -2430,6 +2473,135 @@ def inner(f: T) -> T:
return inner(func)


def private_channel_only(func: Optional[T] = None) -> Union[T, Callable[[T], T]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should either drop the ability to optionally use parens or add the necessary overloads here otherwise you'll get type errors.

.. code-block:: python3

@app_commands.command()
@app_commands.allow_contexts(guilds=False, dms=False, private_channels=True)
Copy link

Choose a reason for hiding this comment

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

Suggested change
@app_commands.allow_contexts(guilds=False, dms=False, private_channels=True)
@app_commands.allow_contexts(guilds=True, dms=False, private_channels=True)

Didn't you want to set guilds to True in the example as that's what you say in the response message just below

@Rapptz
Copy link
Owner

Rapptz commented Mar 19, 2024

Superseded by #9760

@Rapptz Rapptz closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending PR implements an in-progress or explicitly unreleased Discord feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants