-
Notifications
You must be signed in to change notification settings - Fork 3
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: allow setting the c. menu with cms with no handler and mark them for inspection #60
base: main
Are you sure you want to change the base?
Conversation
…mark them for inspection with a new property called "noHandler"
Co-authored-by: KnorpelSenf <[email protected]>
@@ -507,13 +520,14 @@ export class Command<C extends Context = Context> implements MiddlewareObj<C> { | |||
*/ | |||
public toObject( | |||
languageCode: LanguageCode | "default" = "default", | |||
): BotCommand { | |||
): BotCommand & { noHandler?: boolean } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we create our own BotCommand type? maybe AugmentedBotCommand or BotCommandX (taking inspiration from the hydration plug-in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have something like that indeed with CommandElementals
Lines 37 to 44 in 3b332bc
export interface CommandElementals { | |
name: string; | |
prefix: string; | |
language: LanguageCode | "default"; | |
scopes: BotCommandScope[]; | |
description: string; | |
noHandler?: boolean; | |
} |
Maybe is better to use a Picked type from it in the
toObject
function? I might have gotten a little lazy just adding that way hehehe
In any case, I think its more easy to read the simpler BotCommand & { noHandler?: boolean }
, which implies and directly links to, the official grammy interface for setting commands. Since everywhere toObject
is used is in preparing commands for setting onto the command menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was just looking to be a lit bit more DRY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference between CommandElementals
and BotCommand
is that it's property its called name
instead of command
for clarity and separation, but maybe that can be get rid of and we simply extend BotCommand.
That difference was introduced because, basically: name
its plug-in centered, meaning its nature coming from a simple name string or a name regex turned into string, and command
it's always API-centered.
DRY-wise it make sense to just extend it, and change the relevant parts in the code that use that interface, but in someparts, it would be less readable imo, specially for inspection , like in:
Lines 303 to 305 in a6efb7d
name: local.name instanceof RegExp | |
? local.name.source | |
: local.name, |
and
commands/src/utils/jaro-winkler.ts
Lines 143 to 151 in a6efb7d
const bestMatch = cmds.reduce( | |
(best: CommandSimilarity, command) => { | |
const similarity = JaroWinklerDistance(userInput, command.name, { | |
...options, | |
}); | |
return similarity > best.similarity ? { command, similarity } : best; | |
}, | |
{ command: null, similarity: 0 }, | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be less readable imo
because we'd have command.command
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, but it's used so scarcely around the code that we could just make a BotCommandX
As discussed in the telegram group
allows for stuff like:
changes: