Adding bash completion and helper scripts #1993
Conversation
|
Thank you! /cc @albers |
|
+1 for |
|
I'll add my +1 as well.... |
|
I wonder if the helper scripts (prompt, eval) should be in the "completions" directory or moved somewhere else inside "contrib"; I can imagine some people wanting the completions installed, but not those helpers |
|
@thaJeztah I debated that. The prompt function is harmless if you don't put it in your The advantage of keeping them together is that if someone wants completion, they probably want the other shell stuff as well. Similarly, if they disable completion then the probably don't want the other shell bloat. FWIW, Git put its prompt script in its completion directory, which made me feel better about doing it this way. |
|
@nathanleclaire wdyt? |
|
Also, #1309 was attempted before. Anything to merge from there? |
|
@leedm777 thanks, I'm not a maintainer in this repository, and no strong opinion, apart from trying to keep consistency between the various docker repositories. Just thought I'd mention it. Thanks so much though, for creating this! |
|
I'm interested (and thanks for putting this together @leedm777 !), need a chance to review a bit more thoroughly, but two things stand out as needing improvement:
|
|
@nathanleclaire I've added docs, and changed |
| @@ -57,6 +57,21 @@ to your PATH. | |||
| $ docker-machine -v | |||
| machine version 0.5.0 (3e06852) | |||
|
|
|||
| ## Installing bash completion scripts | |||
|
|
|||
| There are a [number of scripts](../contrib/completion/bash) which add command | |||
There was a problem hiding this comment.
I'm not sure if this link is correct - @moxiegirl can you comment? It's meant to link to source code in the docker machine repo.
There was a problem hiding this comment.
Likewise, would be good to get your input on this doc in general.
There was a problem hiding this comment.
@nathanleclaire The link work when viewed on my fork. I'll gladly take recommendations if there's a better way to link to the scripts.
There was a problem hiding this comment.
@leedm777 When you run make docs to validate your changes --- you are doing this right? :-D --- it reports an error on your link.
...
ERROR: 2015/10/22 LinkResolver: No page found for "../contrib/completion/bash" on page "machine/install-machine.md".
...
Soon our own @SvenDowideit will add this to the CI and such things will be caught when you submit your PR. For now, you have to run make docs. In this case, you wanted to link to the contrib directory in the repository. So you would need to actually put this link in https://github.com/docker/machine/tree/master/contrib/completion/bash as a direct link.
@nathanleclaire I'm checking the added content. Thanks for the ping.
|
Putting |
|
@thaJeztah Thanks for pointing me to this, and sorry for the delay. |
|
@albers thanks for looking; I am happy already with you just giving it a quick look to see if there are possible issues or inconsistencies with the other projects 👍 |
| @@ -57,6 +57,21 @@ to your PATH. | |||
| $ docker-machine -v | |||
| machine version 0.5.0 (3e06852) | |||
|
|
|||
| ## Installing bash completion scripts | |||
There was a problem hiding this comment.
@leedm777 Nicely done. I would do a few things to sort this. Put your link OUT last --- no sense inducing them to switch channels before you've explained yourself. Break out the functionality into a scannable list.
---> https://gist.github.com/moxiegirl/9b2dba37dc8e75f5654e
Installing bash completion scripts
The Machine repository supplies several bash scripts] that add features such as:
- command completion
- a function that displays the active machine in your shell prompt
- a function wrapper that adds a
docker-machine usesubcommand
The docker-machine function executes eval "$(docker-machine env X)" in the current shell.
To install the scripts, copy or link them into your /etc/bash_completion.d or
/usr/local/etc/bash_completion.d file. To enable the docker-machine shell prompt, add $(__docker-machine-ps1) to your PS1 setting in ~/.bashrc.
PS1='[\u@\h \W$(__docker-machine-ps1)]\$ '
You can find additional documentation in the comments at the top of each script.
|
@moxiegirl Is that better :-) |
|
@leedm777 thank you. Very glad to help your really helpful contribution! LGTM |
| @@ -57,6 +57,27 @@ to your PATH. | |||
| $ docker-machine -v | |||
| machine version 0.5.0 (3e06852) | |||
|
|
|||
| ## Installing bash completion scripts | |||
|
|
|||
| The Machine repository supplies several `bash` scripts that add features such | |||
There was a problem hiding this comment.
Let's move the link to the contrib/completion/bash section of the repository to this line, i.e. "supplies several bash scripts" should be the link to there.
There was a problem hiding this comment.
Ah, I see it's the opposite of what @moxiegirl recommended. No need to change it then.
|
OK looks pretty good, I have a few docs nits, and just one more suggestion for the UX: If you type |
This patch adds some bash helper scripts. * docker-machine.bash - command completion for docker-machine * docker-machine-prompt.bash - function for putting the active machine name in PS1 * docker-machine-wrapper.bash - function wrapper adding an `use` command that runs `eval $(docker-machine env whatever)` in the current shell. Signed-off-by: David M. Lee <dlee@respoke.io>
|
I think I've addressed all the feedback. This history was getting ugly, so I rebased on master and squashed to a single commit. Thanks to everyone for all the feedback! |
|
Great work @leedm777 ! |
|
LGTM |
Adding bash completion and helper scripts
|
OK, so there won' be any more need for my feedback. |
|
Sorry for that @albers, I wasn't aware @nathanleclaire was gonna merge before you had the chance to give feedback 😊 |
This patch adds some bash helper scripts.
name in PS1
evalcommand that runs
eval $(docker-machine env whatever)in thecurrent shell.