Open dkim286 opened 3 years ago
Hey, thanks for the PR, thinking through the problems here, and the in-depth explanation of the code. Overall it seems pretty reasonable, but it feels awkward to have a mismatch between the default command trigger (.
) and the command trigger that appears in help messages (@
).
I wonder if maybe a better option for the syntax is to take the arguments as a new decorator (e.g. @syntax("[command]")
or maybe even some magic to do @help.syntax()
) and then build the "Syntax: .command" part of the message programmatically. The only real downside I see here is a very slightly less clear syntax when reading the plugin source code.
And a minor difference to note would be that the first command passed to the @command
decorator would be used. Currently, it's possible to have, for example @command(["wolfram", "calc"])
with a @help("Syntax: .calc [query]")
. With this change, you'd have to put "calc" first but that seems completely reasonable.
What do you think?
As a slightly related aside:
A longer-term solution that I've considered in the past is to actually make Cardinal a bit syntax-aware, and respond to invalid syntax for commands itself. This would help reduce some of the boilerplate when parsing a command inside a plugin. I'm not exactly sure what the decorator would look like there. @syntax(param_name, default_param_name="default", *args)
maybe? Core would have to do some clever parsing (e.g. understanding quotes to allow spaces, etc.) Perhaps it even looks the same as it does today and the core performs string parsing to understand the requirement.
I definitely agree that the trigger appearing differently in help message is less than ideal. In hindsight, I really wonder what I was thinking when I went with that decision.
Creating a separate @syntax
decorator was actually the first thing I tried to do while tackling this issue. It didn't quite work out for reasons that I can't remember fully, but here are some tattered remains of what I do remember:
@syntax
decorator would have to be aware of the user-defined trigger in order to:
help
plugin to get the command's name from PluginManager
(or was it the other way around?) was not trivial for some reason.Having said all that, I definitely agree that having it implemented as a decorator is the right way to go about doing it. I just couldn't make it work. PEBKAC and all that. The changes are subpar as they stand now and I'm okay with it being rejected.
Also, I think some of the source code ambiguity can be removed by picking the right name for the new decorator.
@command(["cmd", "command"])
@syntax("arg1 arg2") # potential to confuse arg1 for the name of the command to use
@help("A command that does something really funny")
def command(self, ...):
compared to
@command(["cmd", "command"])
@arguments("arg1 arg2")
@help("A command that does something really funny")
def command(self, ...):
but it could take on any other names as well.
Core would have to do some clever parsing (e.g. understanding quotes to allow spaces, etc.) Perhaps it even looks the same as it does today and the core performs string parsing to understand the requirement.
I would have to understand Cardinal's core better (and Python, really) before I can offer useful commentary on this one. Sorry.
@dkim286 The @arguments()
name is a good one, thanks for the idea. I'll try to find an evening to build on this PR and go that route (you're right that there'll be a bit of trickiness in the help
plugin that probably requires some core changes to get all the necessary info.)
As a note for myself, I'm considering a backwards compatibility layer that parses existing @help
messages looking for "Syntax: ." prefixes and converting them at least until the next major Cardinal release. I can obviously update the plugins in this repository, but not external repositories.
Attempting to solve issue https://github.com/JohnMaguire/Cardinal/issues/189
Here's how the prefix is passed between the modules and where it's being kept, in a (more or less) step-by-step list:
cardinal.py
attempts to loadcmd_prefix
from the config file, if it exists. Defaults to.
if it doesn't exist.CardinalBotFactory
containscmd_prefix
as an instance variable.CardinalBot
reads the prefix from the factory, then passes the prefix toPluginManager
(https://github.com/dkim286/Cardinal/blob/c0786e728a139f0b77e59e0b9fef6d9cf776888d/cardinal/bot.py#L124-L128).PluginManager
uses the prefix to build itsCOMMAND_REGEX
instance variable (https://github.com/dkim286/Cardinal/blob/c0786e728a139f0b77e59e0b9fef6d9cf776888d/cardinal/plugins.py#L61-L63)_instantiate_plugin()
function checks if a module accepts acmd_prefix
argument (https://github.com/dkim286/Cardinal/blob/c0786e728a139f0b77e59e0b9fef6d9cf776888d/cardinal/plugins.py#L189-L203).cmd_prefix
from the plugin manager is aware of the custom prefix (e.g.help
: https://github.com/dkim286/Cardinal/blob/c0786e728a139f0b77e59e0b9fef6d9cf776888d/plugins/help/plugin.py#L11-L12).Unfortunately, this does mean that the syntax help for each command had to be changed slightly. I went with the
@
symbol. For example,help
plugin's command:The "placeholder" token is replaced from within the
help
plugin with a string replace function (https://github.com/dkim286/Cardinal/blob/c0786e728a139f0b77e59e0b9fef6d9cf776888d/plugins/help/plugin.py#L87-L93):I went ahead and changed all the plugins' syntax help messages to use the
@
symbol.It does seem to work for most commands. I have not tested this with the
ticker
module, as I don't have an API for it.Screenshot of the bot in action, default configuration:
Screenshot of the bot in action, configured to use
!
for prefix: