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

Implement slash command and new webhook endpoints #64

Merged
merged 80 commits into from
Apr 16, 2021

Conversation

JohnnyJayJay
Copy link
Member

@JohnnyJayJay JohnnyJayJay commented Jan 1, 2021

This PR makes the following changes:

  • Add new webhook message endpoints
  • Add new slash commands/interactions feature
  • Refactor some internal code

There are some things left to do and clean up:

  • Confirm that application id is a major variable (I am pretty certain about this, but I have not checked myself yet.)
  • Decide what to do with now-required allowed-mentions param in new endpoints
    (positional or optional for consistency with old endpoints?) - right now it is implemented to be consistent with the old code
  • Check what the endpoints return that haven't been fully documented yet (marked as todo in code)
  • General testing. There may very well be some things I missed when writing the code.
  • Write changelog entries

Unfortunately my testing capabilities are rather limited until 2 weeks from now or so.
I can respond to requests for changes already though, so feel free to review.

@JohnnyJayJay JohnnyJayJay marked this pull request as draft January 1, 2021 19:49
@JohnnyJayJay
Copy link
Member Author

Since this has been silent for a while, I'd like to note that the two main remaining issues now are:

1. How to handle parameter names and spec
Currently, the parameter names of the actual functions you call deviate from the names defined in the spec. The reason for that is the following:

All spec definitions are located in the same namespace (and currently need to for the defendpoint macro to work as intended), so it makes sense to give them unambiguous names. For example, when responding to an interaction, you can set a parameter called data in Discord's API. Obviously data isn't a very clear name when thrown together with every other spec so I decided to name it interaction-application-command-callback-data (which is the name of the type of that parameter). But since the field in the actual body will be called data, I used that name in the dispatch definition.
So the question I suppose is what to do with ambiguous field names. This might be a good opportunity to think about a spec namespace restructuring, but since that's not trivial, it may be more appropriate to find a quick workaround. Maybe I can actually use all the ambiguous names for now without clashing with existing spec, but I'll need to check that again.

2. Application ID
Nobody seems to know if the application id is a major variable in the new endpoints or not. I originally wanted to test this myself, but it requires more effort than I thought it would and I haven't found a good way to check this yet. This is probably irrelevant in most cases, but it would definitely be good to know for the implementation detail. So far I've assumed it is a major variable in the endpoints where it is used (some don't even have normal rate limits, I think, but that won't break anything).

@IGJoshua
Copy link
Collaborator

IGJoshua commented Jan 22, 2021

Leaving this here as a checklist for myself:

  • Walk through all endpoints to ensure that existing names match discord documentation
  • Update defendpoint macro to allow specifying a custom spec name
  • Ensure all existing parameters are using known good specs

@IGJoshua
Copy link
Collaborator

My honeymoon is nearly over, and when I get back I plan on working through this PR. Hopefully this can be in a good state soon!

@JohnnyJayJay
Copy link
Member Author

I've thought about a possible solution to the specs issue. Perhaps you could allow putting the endpoint name in the namespace of a spec.
I.e., by default it will work just like now, but if there is no spec with the discljord.messaging.specs namespace, it will also look for one with discljord.messaging.specs.endpoint-name. That would be minimal effort, not break any existing code and eliminate ambiguity where needed.

@JohnnyJayJay
Copy link
Member Author

JohnnyJayJay commented Mar 27, 2021

Following up on (some of) the issue of rate limits:
discord/discord-api-docs#2701

@JohnnyJayJay JohnnyJayJay marked this pull request as ready for review April 5, 2021 17:58
@JohnnyJayJay
Copy link
Member Author

FYI, the permissions feature for commands was merged. It doesn't seem to be on the website yet, but it's in discord/discord-api-docs master.

@JohnnyJayJay
Copy link
Member Author

discord/discord-api-docs#2410 latest comment describes 2 new endpoints to get existing webhook messages and the original interaction response. Will be adding that shortly after testing the most recent changes (file uploads) and implementing them for the create interaction response endpoint.

@JohnnyJayJay
Copy link
Member Author

All slash commands features should be implemented and functional now.

@IGJoshua
Copy link
Collaborator

Awesome, I'll do a little review this morning and see what the status of permissions are on discord's documentation, but we should be good to merge soon.

@JohnnyJayJay
Copy link
Member Author

JohnnyJayJay commented Apr 14, 2021

Docs are still lacking.
Docs for permissions are in the discord-api-docs repository, but still not on the live website. Other than that, attachments for initial interaction responses and the two most recent endpoints (those went live only about 12h ago) aren't documented yet.
Oh and also, remember to change the target branch to feature/slash-commands or something.

Copy link
Collaborator

@IGJoshua IGJoshua left a comment

Choose a reason for hiding this comment

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

This looks really good in all, but there's a couple minor changes.

src/discljord/messaging.clj Show resolved Hide resolved
src/discljord/messaging/impl.clj Outdated Show resolved Hide resolved
src/discljord/messaging/specs.clj Show resolved Hide resolved
@IGJoshua IGJoshua merged commit 371e6f2 into discljord:develop Apr 16, 2021
@JohnnyJayJay JohnnyJayJay mentioned this pull request May 9, 2021
6 tasks
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.

None yet

2 participants