Completion for non invoke programs

[nhoening]

This refers to issue 301.

[bitprophet]

Thanks for this!

Notes in no particular order (with checkboxes so I don't lose track :)):

• There appear to be conflicts in the diff now, so one of us will need to take a look there. Guessing not major ones?
• I'm not sure I agree with merging the non-.py completion files and completion.py into invoke/completion/:
  • I don't like mixing Python source files with other, non-source files. Yea, they're arguably related, but still - I habitually expect folders with __init__.py to contain only Python source, with "data files" living elsewhere in the tree.
  • There's still only the one Python file in that folder/package after your change, so even from a pure Python perspective, it doesn't make a lot of sense.
  • That said, on review, I think it might be semi necessary in order for the files to be present at installed runtime.
  • However, I think we actually need to modify setup.py a bit so that actually occurs on a non-git install (otherwise, an sdist/wheel will only include the .py files)...something something data_files something? Been a while since I used that stuff.
• Wondering if it makes sense to turn the completion scripts into actual templates (vs the semi hardcoded search and replace for invoke strings)...cleaner, but also means an extra step to get the previously "built-in" completion (and/or having to remember to regenerate & re-checkin the result). Thoughts welcome. I think I am OK with how you're doing it now, for the time being...
• I think some of the doc/string language might need to be clearer re: only needing --print-completion-script for custom programs (i.e. vanilla Invoke users can simply refer to the default ones). I can handle this at merge time.
• Some other minor style nitpicks like use of % string formatting, which I again can handle when I merge.

[nhoening]

Hi thanks for replying.

• I checked conflicts last week and they only concern two lines of comments, no code.
• I can look into the adjustment for setup.py so that all files are delivered, good point.
• I have switched jobs and have very little time to do any larger work on this at the moment, so turning the completion scripts into actual templates is out of scope for me. It seems you are okay with the way it is for now, anyway :)
• Good point about clearer doc/string language. I can have a go at that.
• Do you have a style policy about string formatting in this project? Last time I checked, the community in general has not decided for either .format or %, but as a project maintainer you have the right to, of course ...

[bitprophet]

I'm definitely in favor of .format()!

[nhoening]

I did the following:

• resolved the conflicts
• %s -> .format
• added completion scripts as package_data (according to my research, this does the trick, my local tests did not suggest otherwise but maybe I did not test the right way of packaging/distributing)

I did not:

• change the comments to suggest that this is only useful if you run a command that is not called "invoke". In my opinion, the way I documented (source <(invoke --print-completion-script bash), where invoke can be the different name) is the one way for everyone who wants tab completion. One way means simple documentation. It cannot be shorter than this line, either. I believe it also got easier for invoke users than it is now, as currently, people need to find the files themselves. Feel free to correct me if I see this wrong, of course - but it is here where I see this PR improving everyone's tab-completion experience.
• make templates out of the completion scripts. I agree that would be nice, but I might not find the time soonish. I suggest we make a ticket for that.

I hope this is much closer to being mergeable now :)

[bitprophet]

Thanks for the update! Also 👍 about the comments, I probably missed something when I first reviewed, wasn't aware of the suggestion to do the inline source; I agree that it's best to have a cohesive approach regardless of binary name.

[nhoening]

@bitprophet So my idea is that we have sorted out everything - can my work be merged anytime soon? Soon, it will be a year since I did it ... FYI, I just resolved a small merge conflict that came about because master of course keeps changing. It was only about import statements.

[bitprophet]

Finally back on this now...checklist for self:

• Rebase on latest master
• Blacken/flake8
• Get tests passing again
• Proofreading pass
• Enhance tests a bit (they only check for a few small slugs, would feel safer if they hit more of the scripts being printed)
• Decide if I'm ok with the now-baked-in emphasis on pseudo-regex for Program(binary=xxx), or replace it otherwise
  • IIRC prior to this changeset, that was purely intended for human consumption/help display, but is now being treated as a serialized format, which makes me uncomfortable.
• Non-source install test / making sure I understand how package_data works
• Real world test with eg Fabric
• Extend real world test to 2+ Invoke-based apps in same shell env (eg inv + fab)
• Changelog entry

[bitprophet]

Re: how inv[oke]/Program(binary=xxx) was and is used...still definitely torn:

• Prior to this patch, it was purely for display to humans in an informational sense, which is why it uses the not-actually-a-regex, POSIX-help-output-style square brackets format, and is otherwise just a string and not eg a list of strings.
• Also prior to this patch, we stripped out the program/binary name from the input to --complete, since completion scripts must give us the full command line being completed for. This was done via re.sub('inv|invoke'), i.e. hardcoded and not derived from binary.
  • This patch still has that need, of course, but has decided to derive the regex from binary.
• New in this patch, is the additional use of binary to arrive at a list-of-strings for use in mutating the existing completion scripts - it search-and-replaces inv/invoke with the desired short/long names.
  • The code does deal gracefully with binary values which are a single name and have no square brackets.
  • The SnR is being done in lieu of actual "template rendering", which at the time of last discussion we'd decided was probably not worthwhile.
  • The replacements all deal very specifically with how the scripts are written around Invoke's happenstance of "one short, one long, name", which makes this hard to scale up if anyone ever cares.
    • The likelihood of a user having a legit need for more than these two names seems low, but of course nonzero.

What makes me uncomfortable is that we're now treating "your binary name or names" as "a string that is formatted this or that way", even though what we truly want out of it is some list of names. Technically, what we need is one name for use in calling <program> --complete and then a list of 1..N names to give as positional args to the shell's completion mechanism.

(There's also the issue of replicating data supplied in setup.py for entrypoints, but, ehhhhh that's even harder to wrangle.)

---

Anyway, I think this can be (mostly) cleanly solved by:

• Using templates after all; it would actually be less code than the current search and replace, just {0} for main name and {1} for space separated list of all names?
• Modifying Program API to take an extra arg, e.g. binary_names, a list of strings. (Leaving binary as just the help-facing string.)

The downsides:

• Now there are two separate but very similar inputs to Program (on top of name, which is for another similar but distinct display angle)
• If we wanted to make operating w/o that boilerplate a possibility, we'd be doing some weird "find common substring prefixes" code to arrive back at an inv[oke] style string automatically, which strikes me as probably not that much work, but with an extra bug/edge case surface area.

For now I think it may be worth the "two arguments" version, just to avoid releasing and then supporting a "stringly typed" input. Can always do that substring prefix crap later if we really care.

[bitprophet]

Ran into an issue with the real world test – one now wants to run the program to generate completion, but if there's no tasks file present when one does this, one gets the ol' Can't find any collection named 'tasks'!.

At least in my case, I've got no tasks file in my homedir, so putting this in a login dotfile quickly backfires (compared to what was happening before, where the script was static).

The script itself does not need a valid tasks file present in order to work right, since it is itself dynamic (kinda the whole point, vs baking in a static snapshot of one's tasks/args periodically). So it wants to be capable of running w/o any tasks file, similar to --help and --version. Hopefully just a matter of reordering some code. EDIT: dear reader, it was!

[nhoening]

Congrats on finally getting this done!