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

Catch all response sc.15.4.16 #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Catch all response sc.15.4.16 #109

wants to merge 2 commits into from

Conversation

SimonAropama
Copy link

At present there is no default message when a command is not recognised so it fails silently. It would be more user friendly to send a simple message to inform the user that the command was not recognised so that they can check and try again.

A CatchAll can be added via an additional Coffee script but I found that once hubot-stackstorm was installed it only worked with general messages and not those directed to Hubot.

@emedvedev
Copy link
Contributor

Hi! Thank you for the submission.

I can potentially see a problem with it: since hubot-stackstorm uses a catch-all, the "not found" message will probably fire on any valid command that's not part of stackstorm but a separate Hubot plugin. Since we have a number of users who integrate hubot-stackstorm into their own Hubot setup, or use additional modules with st2chatops, I can't merge the PR.

A better way to design it would be this: instead of making hubot-stackstorm fire on catchall, you can get a list of regexes that's built when StackStorm commands are loaded, and only register them as commands. That would involve some work, but this way an external catch-all script will work for sure.

@SimonAropama
Copy link
Author

Ok, presumably during our testing it didn't fire on other commands (e.g. !help etc) as the hubot-stackstorm plugin is loaded last.

Is the suggestion regarding regexes and only registering them something to be done in this script?

@emedvedev
Copy link
Contributor

Yes, you could try to replace this catch-all (that makes hubot-stackstorm fire on every command addressed to Hubot): https://github.com/StackStorm/hubot-stackstorm/blob/master/scripts/stackstorm.js#L297
With a list of regexes. The commands are fetched here: https://github.com/StackStorm/hubot-stackstorm/blob/master/scripts/stackstorm.js#L183
And here's where you get a regex for each format string: https://github.com/StackStorm/hubot-stackstorm/blob/master/lib/command_factory.js#L80
You'll also have to deal with edits/removals of aliases on reload, but it's not too tricky. Of course, it might be a bit of an overkill for your case, but it would be a very welcome contribution.

@mbrannigan
Copy link

bump? 👍

@CLAassistant
Copy link

CLAassistant commented May 11, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ upnica
❌ SimonAropama
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

5 participants