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

Bot command processing #363

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Bot command processing #363

wants to merge 5 commits into from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Oct 11, 2021

This PR is largely a repurposing of https://github.com/Half-Shot/matrix-github/blob/master/src/BotCommands.ts. Notably, it's a class which you bind onto another class that offers command handlers. The idea being that it's then relatively trivial to add new commands, you just add metadata to a function and the rest works.

As an example: https://github.com/Half-Shot/matrix-github/blob/master/src/Connections/GithubRepo.ts#L208-L231

@Half-Shot Half-Shot requested a review from a team October 11, 2021 16:40
@Half-Shot
Copy link
Contributor Author

N.B it would be nice if the help command supported categories ala the irc bridge.

@tadzik
Copy link
Contributor

tadzik commented Nov 3, 2021

I'm porting Slack command handling to this to see how it fares in practice – will come back with proper review and feedback once I'm done with that – I don't want it to be a component that we ship and then don't use ;)

const botCommands: {[prefix: string]: BotCommandEntry<R>} = {};
const proto = Object.getPrototypeOf(instance);
Object.getOwnPropertyNames(proto).forEach(propetyKey => {
const b = Reflect.getMetadata(botCommandSymbol, instance, propetyKey) as BotCommandMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this? Metadata or a BotCommand?
Please pick a better variable name.

Comment on lines +81 to +92
Object.getOwnPropertyNames(proto).forEach(propetyKey => {
const b = Reflect.getMetadata(botCommandSymbol, instance, propetyKey) as BotCommandMetadata;
if (!b) {
// Not a bot command function.
return;
}
const optionalArgs = b.optionalArgs?.map((arg: string) => `[${arg}]`) || [];
const args = [...(b.requiredArgs || []), ...optionalArgs].join(" ");

content += ` - \`${this.prefix || ""}${b.name}\`${args && " "}${args} - ${b.help}\n`;
// We know that this is safe.
const fn = instance[propetyKey];
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
Object.getOwnPropertyNames(proto).forEach(propetyKey => {
const b = Reflect.getMetadata(botCommandSymbol, instance, propetyKey) as BotCommandMetadata;
if (!b) {
// Not a bot command function.
return;
}
const optionalArgs = b.optionalArgs?.map((arg: string) => `[${arg}]`) || [];
const args = [...(b.requiredArgs || []), ...optionalArgs].join(" ");
content += ` - \`${this.prefix || ""}${b.name}\`${args && " "}${args} - ${b.help}\n`;
// We know that this is safe.
const fn = instance[propetyKey];
Object.getOwnPropertyNames(proto).forEach(propertyKey => {
const b = Reflect.getMetadata(botCommandSymbol, instance, propertyKey) as BotCommandMetadata;
if (!b) {
// Not a bot command function.
return;
}
const optionalArgs = b.optionalArgs?.map((arg: string) => `[${arg}]`) || [];
const args = [...(b.requiredArgs || []), ...optionalArgs].join(" ");
content += ` - \`${this.prefix || ""}${b.name}\`${args && " "}${args} - ${b.help}\n`;
// We know that this is safe.
const fn = instance[propertyKey];

* It should contain at least one `BotCommand`.
* @param instance The instance of the above prototype to bind to for function calls.
* @param prefix A prefix to be stripped from commands (useful if using multiple handlers). The prefix
* should **include** any whitspace E.g. `!irc `.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* should **include** any whitspace E.g. `!irc `.
* should **include** any whitespace E.g. `!irc `.

@tadzik
Copy link
Contributor

tadzik commented Nov 4, 2021

Implemented this in Slack for test run here, and a few things stand out:

  1. Commands are case-insensitive, which slack did for one admin room but not the other – is it configurable? Desirable?
  2. There's no support for documenting command arguments, they only have their names shown
  3. No support for named arguments, which Slack admin room took advantage of
  4. No type checking for arguments (yargs used to provide that, perhaps we should at least provide a replacement?)
  5. No support for detailed help (more verbose than the regular help)
  6. doOauth seems to not work, even though it advertises itself as available (is the case insensitivity playing tricks?):

image

Slack is a bit of an extreme case due to being yargs-based, and its commands could definitely be simplified – but perhaps employing a readymade command line parser is not that bad of an idea? Yargs was not fun to work with, but I've had good experiences with commander recently, and it would cover most of the above bases – perhaps we could plug that in as a backend?

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

3 participants