Skip to content
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 #301

Closed
bitprophet opened this issue Jan 13, 2016 · 7 comments
Closed

Completion for non-Invoke programs #301

bitprophet opened this issue Jan 13, 2016 · 7 comments

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jan 13, 2016

I.e. apps like Fabric v2 which leverage invoke.program.Program.

Presently, my local dev workstation has an item in its dotfiles that says source ~/Code/oss/invoke/completion/zsh. Works great for inv. Doesn't do jack for fab.

The upshot of that Invoke completion script is that it looks to invoke --complete -- <whatever is being tab completed>. If I do the same for fab (with a local fabfile.py - without any fabfile.py, or tasks.py for invoke, things don't work at all -- not great -- needs to be another ticket) it works as well.

So the question is "how should developers using Invoke to drive their own non-inv CLI apps, distribute tab completion helper scripts?". Not sure what works great here - some silly meta-script that generates files like /completion/zsh with the correct per-project binary name(s) substituted?

@nhoening
Copy link
Contributor

@nhoening nhoening commented Oct 27, 2016

I think this is actually crucial for everyone building their own apps. I'm doing this for an in-house tool right now and I love Invoke for this. It took me as a bash/zsh amateur a while but in the end the actual changes are just two little replacements. For the record, here are my changes to the zsh and bash completion scripts (maybe someone can use these until this issue gets resolved):

zsh:

20c20,21
<     reply=( $(invoke ${=collection_arg} --complete -- ${words}) )
---
>     # with a custom app name, `invoke --complete` chokes when working on options
>     reply=( $(mytool ${=collection_arg} --complete -- ${words//mytool/invoke}) )
24c25
< # Tell shell builtin to use the above for completing 'inv'/'invoke':
---
> # Tell shell builtin to use the above for completing 'mytool':
30c31
< compctl -K _complete_invoke + -f inv invoke
---
> compctl -K _complete_invoke + -f mytool

bash:

11c11,12
<     candidates=`invoke --complete -- ${COMP_WORDS[*]}`
---
>     # with a custom app name, `invoke --complete` chokes when working on options
>     candidates=`mytool --complete -- "${COMP_WORDS[*]/mytool/invoke}"`
26c27
< # Tell shell builtin to use the above for completing 'inv'/'invoke':
---
> # Tell shell builtin to use the above for completing 'mytool':
30c31
< complete -F _complete_invoke -o default invoke inv 
---
> complete -F _complete_invoke -o default mytool

Let me know if this is done wrong but I found I had to make these replacements, so that invoke --complete would work as intended. I guess it comes down to this line in complete.py which would need to become adaptive to the custom app name:

invocation = re.sub(r'^(inv|invoke) ', '', core.remainder)

As for an actual solution for this issue, two quick thoughts:

  • As you said, provide a "silly" customisation script which applies a patch similar to the above and which accepts the custom app name as a parameter (e.g. "myapp" for the example above). Why not? Invoke already is supporting the completion in an ad-hoc fashion ("source this if you please"), this would just add an intermediate step.
  • An attempt at more programatic solution would be IMHO if Program might be able to remember a custom app name. Then
    1. complete.py could adapt itself (specifically the line I quoted above) with the custom app name
    2. the 'invoke' executable could support a core option which returns a completion script where only the app-name to run completions against is changed (I imagine that users can run eval $(invoke --completion-script [bash|zsh|fish] [app-name]))

I'm not familiar with the inner workings of Invoke, but I'd be curious if the latter is possible or deemed useful. If so, I might have a go at it. A more lightweight possibility is that only a --completion-script [bash|zsh|fish] [app-name] core option is made, which spits out a script with all the changes from my diffs above.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Oct 27, 2016

Thanks for the feedback / diff!

I don't have time to rummage around in Program right now, but I thought it already had some field(s) for "what's the name you're usually bound to on the CLI?" - if it does, or if it doesn't and we add one, then yea, a --print-completion-script or whatnot option might be a semi easy one-shot.

@nhoening
Copy link
Contributor

@nhoening nhoening commented Oct 28, 2016

I hope I can find some time to look into it! (but the next weeks are packed)

You are correct, Program has a property called binary. And --print-completion-script is a better name than the one I proposed.

@nhoening
Copy link
Contributor

@nhoening nhoening commented Dec 19, 2016

@bitprophet I think the PR is now ready to be reviewed/merged. Let me know what I could do in the case it needs improvement.

@bitprophet bitprophet added this to the 1.0 milestone Dec 19, 2016
@nhoening
Copy link
Contributor

@nhoening nhoening commented May 24, 2017

@bitprophet it seems you are finding some time for invoke at the moment. Any chance my PR for this can be merged in the near future? I can live with my workaround, but to have this supported natively would be nicer. I noticed there is a conflict now but it only concerns two lines of comments. I notice you want to do other work on completion (#339), so merging might become complicated if they both are developed in parallel (I happened to propose to move a the completion folder into the invoke package, that would be my major concern here).

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented May 31, 2017

Thanks for keeping an eye on related tickets - at the moment I've got other higher priorities so not too worried about parallel work on #339 :) but good looking out! Will respond to the PR in its own ticket.

@bitprophet bitprophet modified the milestones: 1.0, 1.3 Jun 28, 2018
@bitprophet bitprophet closed this in 40e4ee1 Aug 7, 2018
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Aug 7, 2018

Merged #414! FWIW the changes are largely orthogonal re: #339, tho now that there's some light templating in the scripts, maybe that can impact it, not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.