pyinvoke / invoke

Pythonic task management & command execution.
http://pyinvoke.org
BSD 2-Clause "Simplified" License
4.4k stars 367 forks source link

--help isn't discoverable / cannot give --help after task names, only before #205

Closed dlanger closed 7 years ago

dlanger commented 9 years ago

I have a number of tools built on invoke that take a number of arguments, and these two things still trip me up when I forget them:

  1. That the way to access the help on a task is inv --help taskname (as opposed to most *nix utiliies, where I could do inv taskname --help)
  2. That when I get the parameters wrong, invoke tells me 'taskname' did not receive all required positional arguments!, as opposed to showing me the help output to aid in figuring out what I forgot.

For the former, my ideal way it would work is that if a someone does inv taskname --help and help isn't an argument to the task, we show the help and exit (if it is, we treat it like any other argument). I'm thinking this would be in the Executor - is that a good place to start?

For the second, I have an idea for a PR, but it would require passing the collection to the Parser instance (to get access to the docstring). Would this be acceptable, or does that break the separation of concerns you had in mind?

bitprophet commented 9 years ago

The "prefix --help" has definitely been a sore spot. As you've noticed it's due to the "per-task args" parsing design, but I could be persuaded to merge "if it doesn't clash w/ actual task arg, have suffixed --help behave like the core --help flag" functionality.

It could confuse the hell out of anybody who does define def mytask(c, help) (or rather - anybody using Invoke w/ that tasks collection and not expecting it) but I'm guessing that population is lower than the population expecting inv sometask --help to work.

Re: separation of concerns for number 2, we already create Parser with a set of ParserContext objs generated from the collection - do you need more info than is in those for this?

matleh commented 9 years ago

+1

alexsavio commented 8 years ago

Any update on this issue?

nhoening commented 7 years ago

I've also run into this little UX problem. However, my first expectation was different. Invoke-based CLIs are tools with subtasks to me - a well-known example being git, but there are others. So I was thinking of getting help like this:

git help diff

invoke help <task>

I believe this behaviour would model better what invoke-based CLIs are but I'm not sure how hard it is to bend the parser to achieve this.

bitprophet commented 7 years ago

@nhoening That's definitely a common pattern but it's one I've resisted in the past - for tools like git the set of subcommands is largely "static" but for Invoke (or Make or etc) the entire point is for that set to be user-defined and highly dynamic.

In that case, the core program itself automatically injecting entries into the set of tasks/subcommands - entries that the user didn't themselves define - is "magic" and in my experience it's best to avoid magical behavior when reasonable alternatives exist.

All that said, this is the same argument for not injecting --help after task names, so my earlier counter-argument stands: the number of users who will be unpleasantly surprised by the magic, is probably much lower than the number expecting Invoke to work like other tools they've encountered.

And, in this particular case the total frustration level for the former group should be low, vs other "magic" features like Fabric 1's automatic slash/dollar escaping.

Especially since we can always throw exceptions if users accidentally 'shadow' the magic flag/task (i.e. def help() or def foo(help=xxx)) which eliminates one of the most frustrating corner cases that comes up when one has magic around.

bitprophet commented 7 years ago

As noted in the backlink from fabric/fabric#1594, this core functionality in the parser is needed for two similar but distinct things:

Could potentially do both the same way (and also possibly avoid part of the implementation unpleasantness that prevented this from being done originally) by temporarily granting these special flags to per-task parser contexts, then stripping the "core-only" ones like --help back out before returning. Lets us ride on the natural order of things without polluting the final result.

(A downside [though it may be a problem regardless of overall approach] is when the per-task parsing step would otherwise fail, e.g. a task requiring a positional argument, or a user giving --help alongside incorrect flags, since "just slap --help onto any invocation to get help" is a powerful feature to have. Trusting --help/etc to be treated as "just part of the regular process" makes it hard to guard against these sorts of things.)


It's been a while since I had my hands in the guts of the parser so I'll start by writing the overall tests for this, then trying various PoC implementations.

bitprophet commented 7 years ago

Another random thing that just occurred to me: while accidental overlap between global --help and per-task, really-defined-by-user --help is unlikely, it seems much more likely that a user might have some task arg represented by the shortflag -h. So this feature may also require a tweak to the shortflag generation stuff such that it (perhaps just hardcoded-ly) acts like the h shortflag is always "taken" and will never be selected.

Kinda stinks since it will still surprise a user expecting def mytask(height=5): to be invokable as mytask -h 7 (it'll end up being eg. mytask -e 7). Possibly an argument for only having the 'long' version of the flag globally applied? (But I can see that still tripping people up...)

The Problem With Magic™

ploxiln commented 7 years ago

A compromise idea to help with discoverability: change the error message to something like:

'taskname' did not receive all required positional arguments! did you mean inv --help taskname?

bitprophet commented 7 years ago

@ploxiln well ideally I'd like to solve this so that's not necessary and the parser is made smart enough to go "oh, inv takes-a-positional-arg --help? well I see the existence of --help so even tho I got no posarg I'll just ignore that". Pretty sure it's possible, just needs to be something I'm aware of and not ignoring by focusing solely on the base case :)

bitprophet commented 7 years ago

Dumping some notes to self:

bitprophet commented 7 years ago

Leaning towards the "just make it one parser that can be paused/updated/started again" idea, it feels conceptually cleaner and hopefully won't require a lot of hackery.

Currently, pseudocode:

result = Parser(initial, ignore_unknown=True).parse_argv(argv)
core_args = result[0]
contexts = get_task_contexts_from(core.collection)
context_args = Parser(contexts).parse_argv(result.unparsed)

Note how the 2nd parser has no info about the initial context, which is now bad.

I believe instead we can:

parser = Parser(initial, pause_on_unknown=True)
result = parser.parse_argv(argv)
core_args = result[0]
contexts = get_task_contexts_from(core.collection)
parser.contexts = contexts
context_args = parser.continue()
# Or perhaps just parser.continue_with(contexts)

The subtle difference being the continuation of a single Parser object which gets the contexts loaded up partway. It will retain its original initial context handle, and will also know where it left off in the tokenized argv.

bitprophet commented 7 years ago

Got this working; it's not super pretty but it's not super awful either. It will technically work for other 'core' flags but I haven't tested them out yet.

dmulter commented 7 years ago

Did the second part of the original issue ('taskname' did not receive all required positional arguments!) get lost? I think it would be very useful if the help was also displayed for any task that generates this error due to lack of parameters. As it is currently, every time I get the lack of args error, I then immediately type the command again with --help.

bitprophet commented 7 years ago

I think that 2nd part is covered under #449.

dmulter commented 7 years ago

sure does, thx

cupen commented 6 months ago

Sorry, I thinks it should be invoke taskname --help, beacuse it's more *nix style, that's the default case in most scenarios.