cronokirby / alchemy

A discord library for Elixir
MIT License
151 stars 34 forks source link

unknown command handler #34

Closed nhooyr closed 7 years ago

nhooyr commented 7 years ago

For every command module, it should be possible to register an optional unknown command handler that will be called whenever there is a command for which there is no cog matches.

I need this to be able to create dynamic commands that do not interfere with the normal cog commands.

If you are cool with this, I can go ahead and create a PR.

cronokirby commented 7 years ago

I don't really see the benefit of doing this over just have some kind of name for it:

Cogs.def dyn(arg1, arg2)

I guess this would allow you to call this with !arg1 arg2 instead of !dyn arg1 arg2 It's also not possible to do this at a cog level. There can only be one global "fallback" command.

You can also just do

Events.on_message(:dyn)
def dyn(%{content: @prefix <> rest} = message) do
  case CommandHandler.split(rest) do
    ...
end

Which isn't that much of an effort.

We could do either:

  1. Something like Cogs.set_fallback(mod, func), which I don't really like

  2. Have this in the config, which I think is even worse

  3. Add a new event that would get triggered if the prefix is present but the command doesn't match.

I think 3 is the most viable option, but I don't really see this as super necessary. The 3rd one would also make the pipeline a bit less effficient, because atm finding command functions, and event handle functions can be done in parallel, this would require also hitting the event genserver once more. One use case I could see for this is to provide suggestions for a command, but that's easy to do with the example shown above.

nhooyr commented 7 years ago

I'm ok with Cogs.def dyn(arg1, arg2). If anyone wants to use !dyn as an actual command, they would just have to do Cogs.def dyn("dyn", arg1 and so it should work nicely. Why didn't you list that as a possibility?

The problem with events is that I don't know if a Cog already matched the command. In my bot, users can create custom commands with !add <NAME> <TEXT>. The bot will send the text every time someone types !<NAME>. I have some other predefined commands such as !flip. The problem is that they could add a custom command like !add flip <TEXT> and then every time someone runs !flip with anything, they would get different text that they are not supposed to get. I could manually blacklist certain custom commands, but as my command list grows, it would become unmaintainable.

I'd rather my specific cog commands get priority over my dynamic ones.

nhooyr commented 7 years ago

Actually, I think Cogs.def dyn(arg1, arg2) would not be the best name. Whats happening is that its defining a command from the root so what do you think of Cogs.def root(arg1, arg2)?

nhooyr commented 7 years ago

Though I think that if someone tries to use the !root command normally, they would get confused very easily. What don't you like about Cogs.set_fallback(mod, func)?

cronokirby commented 7 years ago

For implementing custom text mappings like that, I'd probably just use an ets table and store it:

Events.on_message(:custom)
def custom(%{content: @prefix <> rest} = message) do
  case String.split(rest, ["  "], parts: 2) |> MyTable.lookup do
    {:ok, text} -> Cogs.say text
    _  -> Cogs.say "no such custom command"
  end
end
cronokirby commented 7 years ago

If you use a dets table, you get persistent storage of these custom commands, seems like a much better system.

nhooyr commented 7 years ago

I think I might have explained my goal poorly. I'm doing exactly what you described already (except with postgresql instead of an ets table).

The Cog commands colliding with my custom commands is my issue.

cronokirby commented 7 years ago

What we should add is a method that returns a map of the commands you have defined, and then you can check for collisions yourself. That or use a different prefix for your custom commands, which would imo, be a better idea. I can see usefulness in defining such a function though.

nhooyr commented 7 years ago

Yea you know what, I feel like this adds too much complexity for little gain. The only issue I have against a different prefix is that it feels inconsistent but it is certainly a much more simple system and I like the isolation. I'll use a different prefix instead.

Thanks for the help!