sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
948 stars 402 forks source link

shlex for plugin commands #1387

Open dgw opened 6 years ago

dgw commented 6 years ago

Enough modules define a structured set of subcommands and positional arguments that it might make the code a bit more streamlined to support giving plugins a parsed version of the command args. (Think admin.py and its set command, the .blocks suite, and so on.)

This is in concept phase for now, but I wanted to follow up my tangential reference to this idea at #1385 with a bit more detail. I reserve the right to bump it further into the future from its starting point as part of Sopel 7, or even drop the idea entirely if it proves to be impractical.

The reasoning is, Sopel's built-in capture groups only handle up to four space-separated arguments (trigger.group(3) through trigger.group(6)). For modules to handle more than that, or to handle arguments containing spaces, they have to jump through some annoying hoops with split()—especially to handle arguments that can contain whitespace. Sopel's implementation doesn't even allow specifying "this and all following tokens", which would make the whitespace case more bearable at least.

I propose that trigger gain a new property (name TBD, but probably argv or something similar) containing a tokenized version of the command's arguments, the "arguments" being trigger.group(2). Something like shlex would be helpful for that, as it handles Shell-like syntax automatically (including quoted strings and such).

To reduce the performance impact, this could be introduced with a decorator (say, @sopel.module.parse_args) and left empty otherwise.

As yet, I'm not sure if full-on argparse functionality would be useful (or practical to implement in Sopel's module API), but it's a possibility.

Exirel commented 5 years ago

In my opinion, shlex has many advantages:

>>> shlex.split('.command arg1 "arg 2" \'arg3\' arg4')
['.command', 'arg1', 'arg 2', 'arg3', 'arg4']

It's fast, it's easy to understand, and it works quite well!

Except for some cases, where quotes are involved:

>>> shlex.split('.command I want to "quote something"')
['.command', 'I', 'want', 'to', 'quote something']

It doesn't know you want to use an uncut string in this case, and the user would have to do this :

[nick] .command \'I want to "quote something"\'

>>> shlex.split('.command \'I want to "quote something"\'')
['.command', 'I want to "quote something"']

As you mentioned, I think it could be optional. So it's a 👍 overall!

Exirel commented 5 years ago

I had time to think about this, and I've mixed feeling.

On some level, it would be nice to have a more strict command triggering, based on proper detection of arguments. It would be nice, too, if a Trigger object would give out its argument as if it was a shell command line.

However, what prevents developers from using shlex themselves? Maybe they don't know about it, so we should write more documentation with usage example of that. The more I think about it, the more a "common pattern for Sopel plugins" sound like a good candidate for a "How To" page somewhere in Sopel's documentation.

(or they are just very lazy, but I don't care for that)

So, yeah: I think we should decline this feature request, and improve the documentation.

dgw commented 5 years ago

Whether an update to documentation or a new attribute on trigger with pre-shlexed arguments (what I was thinking), this just isn't important enough for 7.0. Punted.

Exirel commented 4 years ago

As I mentioned on IRC, I don't want to work on subcommands and command arguments until we drop Py2.7. In particular, I'd like to use some Py3+ specific syntax to achieve smooth transitions.

Also, it's a complicated topic, and I'm not sure I want to risk that in an already packed with stuff Sopel 7.1 version.

dgw commented 4 years ago

I didn't even notice this was still milestoned 7.1 lol

dgw commented 2 years ago

Clearing milestone, as I'm now unsure what advantage we could provide over having the plugin code handle parsing arguments from trigger.group(2) on its own.