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: add in ability to supply custom command matching interface #25

Merged
merged 2 commits into from
Nov 16, 2024

Conversation

kmcgovern-apixio
Copy link
Contributor

@kmcgovern-apixio kmcgovern-apixio commented Nov 15, 2024

currently when using command groups or spaces in commands, commands can leak between groups. This can cause undesired and confusing results.

example:

I have 2 commands. one in a group and one not in a group

package main

import (
	"context"
	"fmt"
	"os"

	"github.com/slack-io/slacker"
)

func hello() *slacker.CommandDefinition{

	return &slacker.CommandDefinition{
		Description: "Echo a message",
		Command:     "hello <message>",
		Examples:    []string{"hello hello"},
		Middlewares: []slacker.CommandMiddlewareHandler{},
		Handler: func(ctx *slacker.CommandContext) {
			message := fmt.Sprintf("Why hello there: %v", ctx.Request().Properties().StringParam("message", "nothing"))
			ctx.Response().Reply(message)
		},
	}
}

func echo() *slacker.CommandDefinition{
	return &slacker.CommandDefinition{
		Description: "Echo a message",
		Command:     "echo <message>",
		Examples:    []string{"hello hello"},
		Middlewares: []slacker.CommandMiddlewareHandler{},
		Handler: func(ctx *slacker.CommandContext) {
			message := fmt.Sprintf("reply: %v", ctx.Request().Properties().StringParam("message", "nothing"))
			ctx.Response().Reply(message)
		},
	}
}

func main() {
	botToken := os.Getenv("SLACK_BOT_TOKEN")
	appToken := os.Getenv("SLACK_APP_TOKEN")
	server := slacker.NewClient(botToken, appToken)

	personGroup := server.AddCommandGroup("person")
	personGroup.AddCommand(hello())
	server.AddCommand(echo())
	server.Listen(context.Background())
}

here is an example conversation. Using @ mentions for ease of testing, but same behavior is seen with registered / commands

kmcgovern

@demobot person hello

demobot

Why hello there: nothing

kmcgovern

@demobot person echo oh no!

demobot

reply: oh no!

kmcgovern

@demobot hello this does nothing

@demobot echo this is also ok

demobot

reply: this is also ok

my expectation would be that /person echo does nothing instead of the current behavior of calling the echo command as it is only registered as a top-level command.

From my investigation into this, it looks like intended behavior from the commander package. The tokenize and match functions are pretty straight forward and I can see why someone might want the current functionality. For my use-case i do not want the current behavior and require no cross-leakage of commands/words. Since Command is defined as an interface in slacker, I thought it would be good allow providing a custom implementation rather than potentially breaking some functionality of the commander package.

@kmcgovern-apixio kmcgovern-apixio changed the title add in ability to supply custom command matching interface feat: add in ability to supply custom command matching interface Nov 15, 2024
@raed-shomali
Copy link
Contributor

Thank you for your awesome work. Could you do me a favor and create an example for your new changes under the examples folder?

@kmcgovern-apixio
Copy link
Contributor Author

@raed-shomali thanks for the quick response! i added an example that's based off of what was done already in slacker.

@raed-shomali
Copy link
Contributor

raed-shomali commented Nov 15, 2024

Great work!

Make sure to update your commit message to start with "docs:"

Copy link
Contributor

@raed-shomali raed-shomali left a comment

Choose a reason for hiding this comment

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

LGTM

@kmcgovern-apixio
Copy link
Contributor Author

@raed-shomali this should be good now! LMK if you need anything else from me on this :)

@raed-shomali raed-shomali merged commit 15e051a into slack-io:main Nov 16, 2024
2 of 3 checks passed
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.

2 participants