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: user apps #1173

Merged
merged 75 commits into from
Dec 13, 2024
Merged

feat: user apps #1173

merged 75 commits into from
Dec 13, 2024

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Mar 23, 2024

Summary

discord/discord-api-docs@1eaa1af :woo:
(+ discord/discord-api-docs@462cddc)

Adds support for all things related to user-installed applications.
Some parts of the API aren't... great. Particularly some of the terminology/naming used is just confusing, and some parts are just completely nontransparent wrt. defaults and fields taking priority over others. It is what it is. /shrug

Issues

A probably non-exhaustive list of things that are still missing, could be improved, or are still uncertain.

API issues/unclear behavior:

  1. contexts does not default to [0, 1, 2] on the api side, despite the documentation's claims.
    • resolved: this shouldn't matter
  2. It's not clear how Application.install_params and .integration_types_config work together. Right now it seems that the latter takes precedence over the former, if set. The dev portal sends both, looks like the API isn't doing any data shuffling here.
  3. Same thing goes for ApplicationCommand.dm_permission and .contexts. I'm not sure if we even want to deprecate dm_permission right now.
    • resolved: deprecated dm_permission; if both are set, we raise an exception. fwiw, if both are sent, the API prefers contexts over dm_permission
  4. We might want to send a fixed default value for contexts instead of the current null, in case the API ever starts choosing for us.
    • resolved: likely not needed, and can be fixed trivially if it ever happens. worse case is that we sync unnecessarily
  5. The authorizing_integration_owners field is weird, and its structure varies heavily based on configuration/context of commands and interactions. It already has a lengthy docstring, but it might be easier to instead split it into two separate properties and add some of our own fallbacks.
    • resolved: the API somewhat improved here, the fields now seems to be provided in basically all situations; the data is part of a new AuthorizingIntegrationOwners class now
  6. message.interaction_metadata.name isn't documented - quote, "well it will still be there, but i dont want to officially document it". It's an optional field, so implementing it nonetheless is probably fine.
    • resolved: decided to remove this attribute

Bugs/Missing features:

  1. This needs feat(interactions): deserialize channel from data #1012, feat: deserialize dm/group channels in interactions #1233. Additionally, feat!: use the guild data provided on interactions #1236 for the guild attribute in guilds without the bot user. And then likely a ton of mocking for guild.me and several permissions-related methods. Still very uncertain about this part.
    • (partially) resolved: all prerequisite PRs have been merged, except the guild one, which I don't plan on including for now, since it has lots of edge cases and potentially far-reaching consequences
  2. The GuildCommandInteraction annotation (which ends up setting dm_permission=False) silently conflicts with the contexts parameter. See 3.
    • resolved: the annotation now sets contexts = InteractionContextTypes(guild=True) instead of dm_permission = False

QOL:

  1. Bot-wide defaults for the two new command fields could be useful, but aren't trivial to implement. This will be a separate PR.
  2. There are no utility decorators yet (like @commands.default_member_permissions()).
    • resolved: added @commands.integration_types and @commands.contexts decorators
  3. Allowing a command e.g. in all contexts is very verbose due to contexts=[InteractionContextType.guild, ..., ...]. An .all() property on the two new enums could be useful, but also feels a bit weird. Might be resolved by 10. eventually.
    • resolved: the enums were turned into flags, which also have .all()
  4. As mentioned, the official naming is weird. "interaction context" vs "installation context"/"integration type" is easy to mix up, and "integration type" in itself just seems... wrong. Considering renaming all integration_types to installation_types (the one combination of these words that the api docs don't use!).
    • resolved: the official naming remains weird. all "integration types" have been renamed to "install types", which feels like the best description of that particular field.
  5. Another followup PR is likely needed to cover smaller issues related to Interaction.guild, e.g. in ext.commands checks.
  6. More naming stuff can be found at various TODOs in the code.

Documentation:

  1. The docs page still needs updates.
    • resolved: updated the documentation to cover integration_types and contexts, and removed the dm_permission part

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • 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, ...)

@shiftinv shiftinv added t: enhancement New feature t: api support Support of Discord API features s: in progress Issue/PR is being worked on labels Mar 23, 2024
@shiftinv shiftinv added this to the disnake v2.10 milestone Mar 23, 2024
@shiftinv shiftinv marked this pull request as draft March 23, 2024 17:25
@DJStompZone
Copy link

  1. As mentioned, the official naming is weird. "interaction context" vs "installation context"/"integration type" is easy to mix up, and "integration type" in itself just seems... wrong. Considering renaming all integration_types to installation_types (the one combination of these words that the api docs don't use!).

I agree that the naming is weird, and it still may change. But personally I feel like naming things similarly but differently compared to the discord docs could just wind up adding unnecessary confusion to an already murky situation. Especially for people trying to search by specific keywords.

disnake/message.py Outdated Show resolved Hide resolved
@onerandomusername
Copy link
Member

This should have a way to automatically apply the same context to every single command. Like a bot default_contexts parameter.

@onerandomusername
Copy link
Member

DM Permission requires a bit more work (shouldn't impact user-facing side, should not actually touch the API) but this was the quickest change to make it no longer touch the API

@shiftinv shiftinv removed the s: in progress Issue/PR is being worked on label Dec 13, 2024
@shiftinv
Copy link
Member Author

Going to go ahead and merge this now; I'm quite happy with the current state, and it's been long overdue.

The only two remaining points are bot-wide defaults for install_types and contexts, plus some potential adjustments related to Interaction.guild in ext.commands checks. These aren't crucial, and will be added in a future (as in, likely within a couple days) PR.

@shiftinv shiftinv merged commit a95ddaf into master Dec 13, 2024
31 checks passed
@shiftinv shiftinv deleted the feature/user-apps-v2 branch December 13, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: api support Support of Discord API features t: enhancement New feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants