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

fix: Handle InteractionResponseDeferredChannelMessageWithSource in slash commands example. Also refactoring. #1045

Closed
wants to merge 3 commits into from

Conversation

telephrag
Copy link

@telephrag telephrag commented Dec 3, 2021

In /responses command InteractionResponseDeferredChannelMessageWithSource was listed in options but was not handled inside handler function. In result when typing command /responses and selecting option "Deffered response..." bot would either crash or go into infinite loop not knowing how to handle request from user.

Moved handler functions and commands definitions to separate files.

@telephrag telephrag changed the title Refactoring and bug fix in slash commands and example. Refactoring and bug fix in slash commands example. Dec 3, 2021
@telephrag telephrag changed the title Refactoring and bug fix in slash commands example. fix: Handle InteractionResponseDeferredChannelMessageWithSource in slash commands example. Also refactoring. Dec 3, 2021
Copy link
Collaborator

@FedorLap2006 FedorLap2006 left a comment

Choose a reason for hiding this comment

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

Hi, everything looks good to me. But want to mention that we don't usually make go modules inside examples. Also I'm not sure if the slash commands example needs a readme, since it could be built with simple go build.

"github.com/bwmarrin/discordgo"
)

func SelectHandler(s *discordgo.Session, i *discordgo.InteractionCreate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a critical problem, but I think this should be renamed to something else, it is quite confusing, since we have select components.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will look into this on weekend.

Copy link
Author

Choose a reason for hiding this comment

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

It's done. Take a look.

Copy link
Collaborator

@FedorLap2006 FedorLap2006 left a comment

Choose a reason for hiding this comment

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

Whoops, missed one thing

Comment on lines 17 to 18
const GuildID string = "Bot token goes here"
const BotToken string = "Guild where the bot will be used"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is a good idea, since most of the time sensitive stuff is put into flags, plus it is easy to replace these values without recompiling the application again.

@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Mar 23, 2022

Hi. Just wanted to check, if everything is okay and if you still want to continue development of this PR.
Because it would go pretty well with recent #1120 and #1129

@telephrag
Copy link
Author

Sorry, no updates. Busy doing other things.

@telephrag
Copy link
Author

Hi, I've started looking into this again. What should I do in presence of #1120 and #1129?

@FedorLap2006
Copy link
Collaborator

Hi, I've started looking into this again. What should I do in presence of #1120 and #1129?

Good to know! Just wait until they'll be merged. However, while you wait, you can integrate the changes from the base branch.

@telephrag telephrag closed this Apr 11, 2022
@telephrag
Copy link
Author

Closed it accidentally. I'm working on it right now and will reopen PR once I have something to commit.

@telephrag
Copy link
Author

Changes from #1129 are integrated. If you decide to get #1120 done first it's fine, I can wait.

Comment on lines +12 to +14
func Login(ds *discordgo.Session, r *discordgo.Ready) {
log.Printf("Logged in as: %v#%v", ds.State.User.Username, ds.State.User.Discriminator)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having anonymous handler would be better in this case, since it's just oneliner.

Comment on lines 30 to 31
r := recover()
if r != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
r := recover()
if r != nil {
if err := recover(); err != nil {

r := recover()
if r != nil {
deleteCommands(ds, guildID)
log.Fatal("Error occured inside command handler")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log.Fatal("Error occured inside command handler")
log.Fatalf("Error occured inside command handler: %s", err)

}

func Basic(s *discordgo.Session, i *discordgo.InteractionCreate) {
panic("test")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
panic("test")

"Also, as bonus... look at the original interaction response :D",
})
},
ds, err := discordgo.New("Bot " + *BotToken)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you named the session ds, the convention is usually s

func createCommands(ds *discordgo.Session, guildId string) {
var err error
for i, cmd := range commands {
commands[i], err = ds.ApplicationCommandCreate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're doing a refactoring of the example, I think it would be worthwhile to also replace ApplicationCommandCreate with ApplicationCommandBulkOverwrite here. While using Create & Delete is a lot cleaner (you don't affect other commands) and straightforward, they get ratelimited pretty quickly when using them on multiple commands.

var (
GuildID = flag.String("guild", "", "Test guild ID. If not passed - bot registers commands globally")
BotToken = flag.String("token", "", "Bot access token")
RemoveCommands = flag.Bool("rmcmd", true, "Remove all commands after shutdowning or not")
RemoveCommands = flag.Bool("rmcmd", true, "Weather to remove all commands after shutdowning or not")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also speaking of performance and ratelimit issues, I think it would be better to set the default value to false here. That way each time you run the application you won't need to wait until it deletes the commands.

Otherwise, in addition to usage of ApplicationCommandBulkOverwrite, we can replace this parameter with -clean, which when enabled would use ApplicationCommandCreate & ApplicationCommandDelete instead of ApplicationCommandBulkOverwrite. That way it also might make sense to separate functionality into createCommands function instead of doing it directly in main.

}

func BasicWithFile(s *discordgo.Session, i *discordgo.InteractionCreate) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

}

func Subcommands(s *discordgo.Session, i *discordgo.InteractionCreate) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

}

func Responses(s *discordgo.Session, i *discordgo.InteractionCreate) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@FedorLap2006
Copy link
Collaborator

Changes from #1129 are integrated. If you decide to get #1120 done first it's fine, I can wait.

I think yeah, better wait until that one is merged

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