Customized slash command registration #11890

ethervoid · 2019-08-15T17:08:54Z

Summary

This PR includes two new methods in the plugin helpers:

RegisterCommand: This new method allows the plugin creators to register a new command and attach a callback that is executed when the command is launched by a user
ExecuteCommand: This method complements the previous one. This method parses the arguments, generates a new CommandArgs data object and calls the callback that is linked with the received trigger

I'd love to get those bonus points but I've left out the Cobra part by now because I've been struggling with Go and I didn't want to add more complexity to the PR right now :)

I've made the proper modifications to the demo-plugin using these new methods. You can see it here

Ticket Link

Fixes #11843

Related Pull Requests

mattermost/mattermost-plugin-demo#57

These two new methods help the plugin creators to: - reduce the boilerplate in the parameters and trigger extraction. - define a callback for a command in the RegisterCommand that would be executed when the trigger is fired

ethervoid · 2019-08-15T18:19:06Z

@lieut-data Could you please review this PR. I think this is the required changes for the issue but maybe I've misunderstood it or something so I prefer to validate it :)

One question I have. If a command is registered twice, the API.RegisterCommand method doesn't complain and I'm overriding the callback for that command. Is that the expected behavior?

lieut-data

This looks great, @ethervoid! A few small comments.

plugin/helpers.go

plugin/helpers_commands.go

…c convention

…back

…ndle anything

ethervoid · 2019-08-15T20:00:13Z

@lieut-data changes made and I've also included the OriginalArgs inside the new plugin.CommandArgs struct for two reasons:

Reduce the callback parameters
Now we can pass just one pointer to one object in order to retrieve things like UserId, ChannelId and so on that comes in the OriginalArgs

What do you think?

plugin/helpers.go

plugin/helpers_commands.go

ethervoid · 2019-08-16T10:43:47Z

@hanzei Changes done and I've also added more test cases

hanzei

LGTM 👍

plugin/helpers_commands.go

lieut-data

Thanks @ethervoid! Just a few minor comments -- this is looking great!

plugin/helpers.go

lieut-data · 2019-08-16T14:18:03Z

plugin/helpers.go

+	//     func (p *Plugin) ExecuteCommand(c *Context, args *model.CommandArgs) {
+	//         return p.Helpers.ExecuteCommand(c, args)
+	//     }
+	// Returns (&model.CommandResponse{}, nil) if for some reason the callback doesn't exist


I'm not sure it makes sense to describe all the return values here -- this is basically just duplicating the code, and it's very likely to get out of date. Propose removing all the Returns ... comments.

I've seen in the other comments and I just follow a bit the idea. Is ok for me to remove them

plugin/helpers_commands.go

crspeller

Implementation looks good once Jesse's comments are addressed.

However I have to wonder about the usefulness of this. The only operative code is a small block where we parse out the "trigger" from the "args" which is a trivial piece of code that most plugins have already written. I could see this being useful if it provided a full interface like cobra that dealt with all the trivialities of setting up a good CLI, keeping in mind we would really like to support full auto-completion in the future.

Thoughts @lieut-data @ethervoid ?

Co-Authored-By: Jesse Hallam <jesse.hallam@gmail.com>

lieut-data · 2019-08-16T15:30:44Z

@crspeller, agreed that, by itself, this isn't "ground breaking" new functionality, and easily done by plugins themselves.

This is part of the overall epic to expand the plugin helpers and reduce the "boilerplate" that makes it hard to write new plugins. I see this block of code repeated across all of our plugins and, imo, needlessly distracting from the intent and making it just a little bit harder to "get started".

I also see this as a stepping stone towards a cobra-like interface for command handling. While @ethervoid couldn't tackle that part of linked issue in this ticket, I'm expecting to split it out to a separate help-wanted ticket.

Thoughts?

ethervoid · 2019-08-16T16:28:45Z

@lieut-data changes made (waiting for the response of other people to remove the Return comments)

About what @crspeller said. I agree with it but as @lieut-data said, we can split this issue into two so this one covers the base and the new one covers the changes made to introduce cobra.

alifarooq0 · 2019-08-17T04:19:45Z

plugin/helpers.go

@@ -29,8 +33,22 @@ type Helpers interface {
 	//
 	// Minimum server version: 5.6
 	KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error
+
+	// RegisterCommand registers the given command and callback for the ExecuteCommand helper.
+	RegisterCommand(command *model.Command, callback CommandCallback) error


I've noticed the common convention for callbacks in Golang is usually referred to as Handlers, I would recommend renaming this to
RegisterCommand(command *model.Command, handler CommandHandler) error unless there is a good reason not to.

Few examples:
https://github.com/golang/gofrontend/blob/master/libgo/go/net/http/server.go#L1996
https://github.com/golang/go/blob/master/src/syscall/js/func.go#L59

Good for me. I'm a Golang noob :)

crspeller · 2019-08-19T14:58:01Z

@lieut-data @ethervoid But how would we extend this to have that advanced functionality? Using cobra or not? It seems like we would have to add a new helper, adding to the confusion of how to create slash commands. If this where extendable and we had a plan, I would be ok with doing it in stages.

alifarooq0 · 2019-08-19T15:37:16Z

plugin/helpers_commands.go

+		return nil, nil
+	}
+
+	return callback.(CommandCallback)(c, commandArgs)


since p.CommandCallbacks is exposed, is it worth either making it private or asserting the callback is of type (CommandCallback) for better error handling?

That's a good point. I can't create some accessors to the property for example

alifarooq0 · 2019-08-19T18:23:42Z

plugin/plugintest/hooks.go

-import model "github.com/mattermost/mattermost-server/model"
-import plugin "github.com/mattermost/mattermost-server/plugin"
+import (
+	io "io"


were these done automatically through the linter? do we have to alias these?

ethervoid · 2019-08-28T14:53:02Z

@crspeller Sorry for answering that later but I've been out for some days. That said, I can include the Cobra part because it looks like it's mandatory for this change. It's going to take a while because I'm not sure enough how to include Cobra here but I can talk with someone from the Toolkit team

lieut-data · 2019-08-30T14:17:32Z

@ethervoid, thanks for being willing to push on this. When you have time, I'd be happy to sit down and talk through this in more detail!

mattermod · 2019-09-14T00:00:38Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

mattermod · 2019-09-27T00:00:42Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

hanzei · 2019-09-29T10:25:03Z

Hey @ethervoid,

Just wanted to check if we can help you in any way with the cobra changes?

ethervoid · 2019-09-29T10:35:33Z

@hanzei Hi! I'm going to be out for about 1 month until then i'm not going to be able to take a look at It :(. If you need to tackle It ASAP i'm going to suggest yo release It from me. if not, as soon as i come back ill be glad to continue with It :)

hanzei · 2019-09-29T11:48:15Z

Hi @ethervoid, thanks for the heads up! There is no rush on this ticket, so we can keep this open. Looking forward to you continue your work 👍

lieut-data · 2019-11-28T16:18:37Z

I synched up with @ethervoid offline, and while we'll keep this PR around for reference, he won't be continuing with this effort right now.

ethervoid added 5 commits August 15, 2019 18:30

Add RegisterCommand and ExecuteCommand to the Plugin Helpers

44506f7

These two new methods help the plugin creators to: - reduce the boilerplate in the parameters and trigger extraction. - define a callback for a command in the RegisterCommand that would be executed when the trigger is fired

Plugin Commands helper tests

dc104e7

Fix test func name for commands helper

0c591c0

Don't store the callback if the command register fails

3acb334

Improve the helpers documentation comments

ca6d6ff

lieut-data requested review from lieut-data and crspeller and removed request for lieut-data August 15, 2019 18:26

lieut-data requested changes Aug 15, 2019

View reviewed changes

ethervoid added 5 commits August 15, 2019 21:35

Change documentation for the new plugin helper methods following godo…

b8f07dd

…c convention

Add plugin.Context as first-class parameter in the plugin.CommandCall…

96fc985

…back

Check first for errors in the plugin.Helpers.RegisterCommand method

923cbfd

Return nil to signal the server that Helpers.ExecuteCommand didn't ha…

8ec7784

…ndle anything

Include the model.CommandArgs inside the plugin.CommandArgs struct

697766a

hanzei added the 2: Dev Review Requires review by a developer label Aug 15, 2019

ethervoid mentioned this pull request Aug 15, 2019

Integrate new plug.Helpers commands (Register and Execute command) mattermost/mattermost-plugin-demo#57

Closed

hanzei reviewed Aug 16, 2019

View reviewed changes

plugin/helpers.go Show resolved Hide resolved

plugin/helpers_commands.go Outdated Show resolved Hide resolved

plugin/helpers_commands.go Outdated Show resolved Hide resolved

plugin/helpers_commands.go Outdated Show resolved Hide resolved

ethervoid added 3 commits August 16, 2019 12:40

Improve documentation for plugin.Helpers.ExecuteCommand

66a73b5

Change code to be more idiomatic and goish

5997cb8

Include more tests for helpers_commands

afb80a7

hanzei approved these changes Aug 16, 2019

View reviewed changes

plugin/helpers_commands.go Outdated Show resolved Hide resolved

hanzei requested a review from lieut-data August 16, 2019 11:03

lieut-data requested changes Aug 16, 2019

View reviewed changes

crspeller requested changes Aug 16, 2019

View reviewed changes

Remove error conditions in the docs

6ac5777

Co-Authored-By: Jesse Hallam <jesse.hallam@gmail.com>

Embed the model.CommandArgs into the plugin.CommandArgs struct

a743b3e

alifarooq0 reviewed Aug 17, 2019

View reviewed changes

alifarooq0 reviewed Aug 19, 2019

View reviewed changes

lieut-data self-assigned this Aug 20, 2019

lieut-data removed their assignment Aug 30, 2019

mattermod added the Lifecycle/1:stale label Sep 14, 2019

hanzei added Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a developer Lifecycle/1:stale labels Sep 14, 2019

mattermod added the Lifecycle/1:stale label Sep 27, 2019

hanzei added Lifecycle/frozen and removed Lifecycle/1:stale labels Sep 29, 2019

amyblais added Hacktoberfest and removed Hacktoberfest labels Oct 1, 2019

lieut-data closed this Nov 28, 2019

hanzei added Lifecycle/3:orphaned and removed Lifecycle/frozen Work In Progress Not yet ready for review labels Nov 29, 2019

RajatVaryani mentioned this pull request Dec 2, 2019

Customized slash command registration #11843

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customized slash command registration #11890

Customized slash command registration #11890

ethervoid commented Aug 15, 2019 •

edited

ethervoid commented Aug 15, 2019

lieut-data left a comment

ethervoid commented Aug 15, 2019

ethervoid commented Aug 16, 2019

hanzei left a comment

lieut-data left a comment

lieut-data Aug 16, 2019

ethervoid Aug 16, 2019

crspeller left a comment

lieut-data commented Aug 16, 2019 •

edited

ethervoid commented Aug 16, 2019

alifarooq0 Aug 17, 2019

ethervoid Aug 28, 2019

crspeller commented Aug 19, 2019

alifarooq0 Aug 19, 2019

ethervoid Aug 28, 2019

alifarooq0 Aug 19, 2019

ethervoid commented Aug 28, 2019

lieut-data commented Aug 30, 2019

mattermod commented Sep 14, 2019

mattermod commented Sep 27, 2019

hanzei commented Sep 29, 2019

ethervoid commented Sep 29, 2019

hanzei commented Sep 29, 2019

lieut-data commented Nov 28, 2019

Customized slash command registration #11890

Customized slash command registration #11890

Conversation

ethervoid commented Aug 15, 2019 • edited

Summary

Ticket Link

Related Pull Requests

ethervoid commented Aug 15, 2019

lieut-data left a comment

Choose a reason for hiding this comment

ethervoid commented Aug 15, 2019

ethervoid commented Aug 16, 2019

hanzei left a comment

Choose a reason for hiding this comment

lieut-data left a comment

Choose a reason for hiding this comment

lieut-data Aug 16, 2019

Choose a reason for hiding this comment

ethervoid Aug 16, 2019

Choose a reason for hiding this comment

crspeller left a comment

Choose a reason for hiding this comment

lieut-data commented Aug 16, 2019 • edited

ethervoid commented Aug 16, 2019

alifarooq0 Aug 17, 2019

Choose a reason for hiding this comment

ethervoid Aug 28, 2019

Choose a reason for hiding this comment

crspeller commented Aug 19, 2019

alifarooq0 Aug 19, 2019

Choose a reason for hiding this comment

ethervoid Aug 28, 2019

Choose a reason for hiding this comment

alifarooq0 Aug 19, 2019

Choose a reason for hiding this comment

ethervoid commented Aug 28, 2019

lieut-data commented Aug 30, 2019

mattermod commented Sep 14, 2019

mattermod commented Sep 27, 2019

hanzei commented Sep 29, 2019

ethervoid commented Sep 29, 2019

hanzei commented Sep 29, 2019

lieut-data commented Nov 28, 2019

ethervoid commented Aug 15, 2019 •

edited

lieut-data commented Aug 16, 2019 •

edited