New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion for non invoke programs #414

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@nhoening
Contributor

nhoening commented Dec 16, 2016

This refers to issue 301.

@bitprophet

This comment has been minimized.

Member

bitprophet commented May 31, 2017

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

This comment has been minimized.

Contributor

nhoening commented Jun 1, 2017

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

This comment has been minimized.

Member

bitprophet commented Jun 1, 2017

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

@nhoening

This comment has been minimized.

Contributor

nhoening commented Jun 11, 2017

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

This comment has been minimized.

Member

bitprophet commented Jun 13, 2017

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.

@bitprophet bitprophet added this to the 0.19 milestone Jun 13, 2017

@nhoening

This comment has been minimized.

Contributor

nhoening commented Sep 12, 2017

@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 bitprophet modified the milestones: 0.21, p2 Jul 12, 2018

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 6, 2018

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

This comment has been minimized.

Member

bitprophet commented Aug 7, 2018

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

This comment has been minimized.

Member

bitprophet commented Aug 7, 2018

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!

bitprophet added a commit that referenced this pull request Aug 7, 2018

Implement overhaul re #414
- templatized completion scripts
- Program.binary_names now live
- tests updated and/or extended
- one actually-pointless test removed

bitprophet added a commit that referenced this pull request Aug 7, 2018

Massive overhaul of docs re #414
Based in part on my screwing around trying to get invoke
(installed globally) and fab (installed only in a virtualenv)
both completing on my workstation

@bitprophet bitprophet closed this in 40e4ee1 Aug 7, 2018

@nhoening

This comment has been minimized.

Contributor

nhoening commented Aug 8, 2018

Congrats on finally getting this done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment