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

Slash commands #856

Merged
merged 31 commits into from Mar 1, 2021
Merged

Slash commands #856

merged 31 commits into from Mar 1, 2021

Conversation

FedorLap2006
Copy link
Collaborator

Here we go! Slash commands are ready to use (almost).
So... I saw that everyone wants it, so I'm opening it now, not when I will complete it.
Now it includes some example in examples/ folder of using this new feature and basic API of slash commands (CRUD).

Well, enjoy!

@eaglemoor
Copy link

any update?

theovidal pushed a commit to BecauseOfProg/XBOP that referenced this pull request Jan 6, 2021
Added support for new slash commands in a very dirty manner. Waiting for bwmarrin/discordgo#856 to be merged so they'll be supported in the official library.
@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Jan 6, 2021

any update?

Well, I'm busy... so, I will try in near days find time for finishing this.

@FedorLap2006
Copy link
Collaborator Author

What can I say... I've just completed slash-commands. But, I'm going to refactor, cleanup code and push it today or tomorrow, so stay in touch.

@FedorLap2006
Copy link
Collaborator Author

Well... as you see.

@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Jan 16, 2021

I'd like to have some code review before merging, but maintainers will decide.

interaction.go Outdated Show resolved Hide resolved
interaction.go Outdated Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
restapi.go Show resolved Hide resolved
restapi.go Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
structs.go Show resolved Hide resolved
Copy link
Collaborator

@CarsonHoffman CarsonHoffman left a comment

Choose a reason for hiding this comment

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

I think this is a great start! An example is a great inclusion for a new feature. There are a few points that I think can be streamlined, but otherwise this is definitely on a great track.

endpoints.go Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
restapi.go Show resolved Hide resolved
@CarsonHoffman
Copy link
Collaborator

Additionally, note the failing check.

@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Jan 17, 2021

Well, for now, I have no time to fix it, but all fixes will be ready tomorrow.

@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Feb 28, 2021

Nevermind, ignore my issue about registering commands. I missed this sentence in the Discord developer docs.

Global commands are cached for 1 hour. That means that new global commands will fan out slowly across all guilds, and will be guaranteed to be updated in an hour.

This might be worth adding to the description of ApplicationCommandCreate just so people don't create issues about this in the future. Unless you read the entire section on registering commands you might be liable to miss that caveat (like I did).

Well, I think I want to hear other people's opinion on this. This is not necessary, but yeah, maybe really nice to have.
Also, wanted to mention, you can actually try to use guild-scope commands, they're almost instantaneously available after registering.

@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Feb 28, 2021

Hope there's nothing more else to do so we can already finish. If not, please mention not yet solved issues, I just... in such a big (by reviews, comments, etc) PR I can simply forget to fix something.

And thanks for understanding, reviews and time you spent waiting for this, one of the greatest Discord's feature to be implemented in DiscordGo. Also, I'm sorry for those whom I could disrespect during the reviews and development of this PR, I know, that was a problem at the start of the beginning, but now, just wanted to say, I was doing this, because I wanted to quicker finish this PR (free myself from developing this and focus on my projects, and allow people to use this), I hope you understand.

@MrFlynn
Copy link

MrFlynn commented Feb 28, 2021

Well, I think I want to hear other people's opinion on this. This is not necessary, but yeah, maybe really nice to have.

Sure, it's not 100% necessary. Just a nice-to-have.

Also, wanted to mention, you can actually try to use guild-scope commands, they're almost instantaneously available after registering.

Yep, saw that when I did a more thorough re-reading of the docs.

@CarsonHoffman
Copy link
Collaborator

CarsonHoffman commented Mar 1, 2021

I've added some additional changes to the PR, mainly to address three areas of concern:

  1. Adherence to the established patterns of the library. These aren't necessarily "better", but fall more in line with the muscle memory of current users of the library.
  2. Upcoming changes announced by Discord. We should try to get out ahead of these with the pace at which slash commands are evolving.
  3. English. I fully sympathize with English not being your first language and don't knock you for this, but given that the comments are going to be public-facing documentation, I did a pass through to clean things up a bit.

I am aware that pushing my own changes to the PR isn't ideal GitHub etiquette. From the 177 previous comments on this PR without much movement, however, I felt as though these needed to be closed out. I'll be the first to tell you that the community is super grateful for your work here, and I didn't want to leave this sitting here for more weeks in a back-and-forth on the issues mentioned in some of the commentary. I think that this solution will speed up the deployment of this feature, and will result in the same end product (that is: if the changes were eventually made; otherwise, they could have jeopardized any merge).

That all said: I feel as though the concerns voiced in much of the discussion have been properly addressed. I am not claiming that this feature is perfect as-is, but the growing community interest means that (easily) getting it into the hands of all of you must be taken as a priority.

To update to a version of DiscordGo containing this feature, run go get github.com/bwmarrin/discordgo@master in your module directory. I am not creating a tag at the moment in anticipation of issues that might come up—please do report any issues that you run into in wider use of the feature so we can continue to iterate on it for the benefit of the community at large. Given no major issues, this will probably drop in a new minor version in a week or so.

That's it folks! Thanks for reading my essay. Now go have some fun!

@CarsonHoffman CarsonHoffman merged commit b0fa920 into bwmarrin:master Mar 1, 2021
@jmsheff
Copy link

jmsheff commented Mar 1, 2021

A big thank you to all who contributed!

@FedorLap2006
Copy link
Collaborator Author

FedorLap2006 commented Mar 1, 2021

Thanks. Finally 🚀
Only one thing: I don't quite think that you're (@CarsonHoffman) right with "Dependence on gateway". I made this feature so you won't pass everything by youself, but instead use automatic ID detection. Anyways, I glad this ended and everyone now can use this.

@FedorLap2006 FedorLap2006 mentioned this pull request Mar 1, 2021
@qaisjp
Copy link
Contributor

qaisjp commented Mar 1, 2021

Great work @FedorLap2006 and co 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Contains breaking changes. Should be reflected in the changelog feature Feature implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet