Core flags given after task names fail silently

[bitprophet]

Thanks to #205 we can now do things like inv mytask --help to get per-task help.

Unfortunately, a bug snuck in (quelle horreur!), namely that other core flags given after task names seem to just...do nothing. For example, inv --echo mytask shows command echoing, but inv mytask --echo does not. No error, but also no echo.

At first I feared this meant that any post-task flags that were invalid were getting swallowed silently, but this doesn't seem to be the case, things like inv mytask --lolno still result in parser errors.

So I think this is probably just a TODO that isn't implemented yet, where we need to add more logic/behavior interpreting the rest of the core flags on a per-task-execution basis.

[bitprophet]

Figured this out just now while poking a related issue; it's simply because the code added for #205 did so in a very backwards compatible fashion (added self.core_via_tasks instead of directly/globally modifying self.args) and then only the "print help/list tasks" part of the code (Program.parse_cleanup) was updated to look at self.core_via_tasks.

The rest of the core arg attending code (i.e. that which sets things like echo; it's in Program.update_config) was not updated and still only refers to self.args - thus why these other core flags disappear into the aether.

Have a fix in place and as usual updating the tests is gonna take a bunch more time than the fix itself 😓

[bitprophet]

Also, wondering if this is truly a bug or an expectations mismatch:

• With --help, the intent was that inv taskname -h implicitly acts like inv -h taskname - where the task name is itself used as the value for -h
• I have skipped tests in place that state the other core flags like --list should actually not work in "task contexts", which goes against this ticket here and possibly the docs (need to check).
• Those together imply this was only ever really supposed to work for --help, which is not what I keep remembering (eg this bug was filed because I keep habitually trying to slap -e at the end of command lines, etc).
• Typically, my gut instinct should be the guide in such situations (if I assume as much, others will too) but I need to see what our docs currently say; if we're really explicit about it and there's a stated reason, maybe we need to go the other way here (delineate --help as being special after all, and explode with parse errors for anything else). I'd rather not tho.

[bitprophet]

The docs appear very specific that this behavior was intended to be for --help; nowhere does it state that it "should" work for any/all core flags. Makes me wonder if anybody else is even noticing this or if it's uniquely a "Jeff problem"; seems likely the latter.

Of course, given it's easily fixable, I still think I'd like to do so, I just gotta tidy up that test suite and enhance the docs.

---

OTOH, my spidey-sense is tingling; this feature has always had the issue of what to do when ambiguity strikes (if a task has a -h, or – should I 'fix' this 'bug' – a -e, or a -D etc). The only good option is what is already done for -h/--help - if the task defines that same flag, the task-level flag wins. (Exploding or having the core flag win, would quickly eat into users' available arg namespace, which is a big no-no for this sort of tool.)

But even given this, the problem is one of consistency: as soon as the user encounters one of those "ambiguous, but the task wins" scenarios, something that used to work (or works on other tasks) stops working. E.g. say everybody gets used to slapping "-e for echo" at the end of the line; the habit is formed, but then as soon as you encounter def foo(env=xxx) or whatever, suddenly your muscle memory is a lie, and you don't get what you habitually expected.

I don't see any solution to that, and so my worry is that if I make "all core args are valid for all unambiguous contexts" a real thing, that inconsistency problem will become more of an issue for the average user.

Or I can take the door that's open to me here, and say that this is actually the opposite bug, calling inv random-task-here -e should result in the usual "no idea what -e is!" parse-error instead of silently eating the -e, and close that off. Fewer conveniences but also more consistency.

---

A counter-counter-argument is: I ended up back here because I wanted to see if it was feasible to implement fab deploy -H hostname (i.e. not requiring users to remember to place the target first and the task second, as is currently required). I still kinda want that. But is it worth the above?

---

If we do go that direction, there is another issue: the implementation of #205 left us with an annoyance in the parser code, where it says "if we're in a task context, and we see a core flag and it takes a value, use the context name as the value instead of looking to the next token". This is why inv taskname -h acts like inv -h taskname.

But it means any other core flag taking a value doesn't work anymore in a task context! E.g. inv taskname --hide both would act like inv --hide taskname taskname (or similar) and discard, or error on, the both.

If this is to be fixed, it needs some other way of marking --help as special, I suspect - it's the outlier here.

[bitprophet]

I suppose another way to look at the "you can slap -e and friends after tasks" is that it's purely a convenience/extra/gimme, in the situations where it's unambiguous. Users should be taught that these mostly belong in the core parse context, and that if they happen to work outside of it, that's a happy coincidence – but one not to be relied upon.

Another argument is simply that there's no putting the inv taskname -h genie back in the bottle, and so this is all merely a matter of degree. Plus, having it work for -h but not -e is (as seen by me filing this ticket to begin with) is its own inconsistency.

There's no clear-cut winning here, so I am thinking we might as well roll with the convenience, as much as it pains the purity-minded side of me.

[bitprophet]

Diving into this and mostly done, I find that yanno...the split between Program.args and Program.core_via_tasks really is dumb and all over the place. I think old-me was in fact being too paranoid when he made core_via_tasks separate.

Given that this parser-exposed behavior was not even possible beforehand, and we're technically changing behavior (albeit from ParseError to non-error behavior), trying not to change the overall value of .args isn't going to actually help anybody subclassing/hacking these classes, avoid at least some pain or surprise.

EDIT: grumble, while there was no attribute-docstring for .core_via_tasks, it is mentioned in the docstring for parse_tasks. So we do need to keep that around for compatibility reasons - though I think we can argue that updating args can be considered a bugfix level change anyways. (It will still be possible to detect weird behavior by comparing individual contexts, args and core_via_tasks, so it's not totally vestigial or anything.)

[bitprophet]

Went halfsies and decided to make this overall a feature-level-bugfix; this way it limits patch level stability issues.

Seem to have actually sorted out behavior & tests (including a potential roadblock in the parser, which was fixed via some #showerthoughts) so this might be done now (!).

[bitprophet]

Ran into some extremely stupid sidequests and still not done with them. The main issue consists of the pseudo-internal APIs in Program and Executor; needed a way to easily represent the union of the "true" core flags (as parsed at the front of the CLI) and the "via tasks" core flags (the new behavior added here).

Initially decided not to modify Program.core[0] (a ParserContext, which is inside a ParseResult) but to instead add Program.core_via_tasks and then update Program.args (a property) so it reflects the union of both (as needed above). This took a while to nail down because there are some finicky edge cases.

Got that all working, only to find out that, oh. This works fine for core Invoke, but for downstream code like Fabric, which does work in a custom Executor that wants access to core flags (specifically in this case, a --hosts flag), the Program->Executor interface is too limited. Program creates Executor and hands it the loaded task collection, the loaded config, and self.core (the core args). (Then hands that instance the parsed task contexts to execute.)

The problem: the new shiny Program.args is thus invisible to Executor and it cannot see the unified core arg values, only the 'regular', given-up-front ones.

---

I'm glad I'm rubber ducking this because I think the solution might actually be easy. I was hesitating to go the obvious route - mutate Program.core[0] after all, in a single step after that final parse - because it felt backwards incompatible.

Then I remembered, well, older versions are arguably broken as-is - users can give the core args after tasks, and nothing happens (or, in even older versions - IIRC pre-1.0 - an explosion occurs because those flags are invalid in that position). Going from a silent error to the data becoming updated isn't a problem. It'd only be a problem if there had been a prior release where this functionality already worked and was implemented in a way that left Program.core unmolested. But no such release exists - it's just in my branch as of today.

Still means I have to shuffle all the new code and tests I wrote today, but, whatever. Wasted so much damn time on this as it is...

[bitprophet]

OK, that's a wrap. Both Invoke and Fabric 2 can give core flags anywhere they like.