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

Add Bash Completion Support #99

Merged
merged 1 commit into from Feb 10, 2016

Conversation

Projects
None yet
3 participants
@nickw444
Contributor

nickw444 commented Feb 2, 2016

I've implemented deep bash completion integration within the library. Work towards #75. A bash completion script can call into any tool using Kingpin by using a new flag --help-bash-completion as the first argument, followed by subsequent user-entered arguments.

Opening this PR to get some eyes on it, and see how it can be improved. Happy to add docs once we're happy with the overall structure of it (Since this does bring some new API):

  • type HintAction func(args []string) []string A new type HintAction, a function that returns a list of options for completions.
  • func (a *ArgClause) HintAction(action HintAction) *ArgClause to register a HintAction to call when completions are requested for an argument.
  • func (a *ArgClause) HintOptions(options ...string) *ArgClause to register a static slice of strings to provide as completions when requested. Internally registers a HintAction that returns this slice.
  • func (a *FlagClause) HintAction(action HintAction) *FlagClause - Same as described for ArgClause
  • func (a *FlagClause) HintOptions(options ...string) *FlagClause - Same as described for ArgClause

Additionally, FlagClause EnumVar will register all available EnumVar options as completion options too.

Users will be presented with completion options for:

  • sub-commands
  • flags for a command (when - was the last user-entered argument)
  • options for a flag (when the second last argument was a flag)

There's a bit of re-shuffling in this PR, including a new mixin (cmdMixin), due to some shared logic between the Application type and the CmdClause type for autocompletion

Additionally, a lot of logic from Application.execute has been moved to Application.Parse to allow different code paths for bash completion / normal execution.

An additional argument has been added to Application.applyPreActions to change whether or not sub commands should have preActions dispatched, since this causes issues with functionality such as help, and --version

Included are two scripts support/bash_completion and support/zsh_completion which should be modified and then sourced/installed by end users within their shells to enable bash completion.

@nickw444 nickw444 referenced this pull request Feb 3, 2016

Closed

Bash completion #76

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 3, 2016

Hi @nickw444, thanks for the PR. I've had a look, but I'm still pondering some design/usability concerns.

  1. Foremost, I think, is that I think (I haven't stewed on this sufficiently to be sure) that I'd prefer if the entirety of the completion script were generated by Kingpin (eg. via a --bash-completion-script flag), rather than having a separately installed script. This is a usability concern. This would also allow the completion implementation to change.
  2. I'm not 100% convinced that calling into the app to do completion is the best approach (vs. generating the full completion script up front).
  3. It would be ideal if flags with values had = appended (eg. --expires= vs. --expires) and didn't add a space. I had a quick stab at this, but couldn't figure out how to tell bash not to add a space after completing.

I really like the hint approach, this could also be useful for eg. generating more useful help.

Anyway, I'll have a bit more of a think about it, but at the moment I'm thinking that if 1. above is addressed, I'll probably just merge it because it's a great start, then if the underlying implementation changes it will be easy to do.

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 3, 2016

Also, I think the hint methods could use some documentation.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 4, 2016

Hey @alecthomas thanks for the comments. I understand your concerns in all of your above points.

1.

I've moved the "support" scripts into the templates.go file and implemented 2 new flags to generate these supporting files, --completion-script-bash and --completion-script-zsh. However, my main concern is with package distribution for owners tools which use kingpin.

Consider this: when distributing a package, (specifically a .deb), the package maintainer will usually add a bash_completion script to the package to be installed in /etc/bash_completion.d. Adding these flags does help, but it will need to be made clear bash completion doesn't come for free - If they want end users to get completions, they need to install these (generated) completion files in their package distributions.

Alternatively, package maintainers need to inform users that they can add additional lines to their .bash_profile and .zshrc such as:

eval "$(my-cli-tool --completion-script-bash)"

However, personally, I feel this isn't a great end user experience. Anyway, with that all being said, it's definitely the right way to go to move the scripts into kingpin, where a package maintainer can generate these scripts after compilation and add them to the distribution.

2.

I originally felt that keeping completions outside of the app was the best idea, as it's more isolated and controlled (plus an easier feature IMO :P ), but, it didn't give users of kingpin great options for handling dynamic arguments without re-generating the script every time.

A good example of such a use case is where you have a CLI tool which reads from an hosts file and provides completion of possible known hosts. By reaching into the application and performing a HintAction we can read new options on the fly without any re-generation. I think this will be made more clear with examples.

3.

I originally attempted this, and also implemented a hybrid, however due to how bash (and ZSH) handle completion selection, a space will always be added (unless there are multiple options to choose from).

The original implementation I speak of would output every possible --<flag>=<possible option for flag> combination, but ended up being very messy in the case that the user was unsure initially of which flag to type (ie, just typing --).

docs

I've added documentation and tests. Let me know if you've got any additional comments :)

@nickw444 nickw444 force-pushed the atlassian:nwhyte/autocompletion branch 3 times, most recently from 34b62a5 to 02d3a22 Feb 4, 2016

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 6, 2016

The tests are failing, but once they're fixed up I'll take another look. Thanks!

app.go Outdated
a.cmdGroup = newCmdGroup(a)
a.HelpFlag = a.Flag("help", "Show context-sensitive help (also try --help-long and --help-man).")
a.HelpFlag.Bool()
a.Flag("help-long", "Generate long help.").Hidden().PreAction(a.generateLongHelp).Bool()
a.Flag("help-man", "Generate a man page.").Hidden().PreAction(a.generateManPage).Bool()
a.Flag("help-bash-completion", "Output possible completions for the given args.").Hidden().BoolVar(&a.completion)

This comment has been minimized.

@alecthomas

alecthomas Feb 6, 2016

Owner

I think I'd prefer this be named ---completion-bash, as it has no real relationship to help.

assert.Equal(t, c.ExpectedOptions, args, "Expected != Actual: [%v] != [%v]. \nInput was: [%v]", c.ExpectedOptions, args, c.Args)
}
}

This comment has been minimized.

@alecthomas

alecthomas Feb 6, 2016

Owner

Nice test, lovely.

@@ -0,0 +1,31 @@
package kingpin
type HintAction func(args []string) []string

This comment has been minimized.

@alecthomas

alecthomas Feb 6, 2016

Owner

Can you document this? What exactly is args? Also, it seems it is unused almost everywhere, if that's the case maybe it could be omitted?

// You can provide hint options statically
nc.Flag("port", "Provide a port to connect to").
Required().
HintOptions("80", "443", "8080").

This comment has been minimized.

@alecthomas

alecthomas Feb 6, 2016

Owner

This is a good example of where HintOptions() could be useful. Maybe put that in the README as well?

"fmt"
"os"
"gopkg.in/alecthomas/kingpin.v2"

This comment has been minimized.

@alecthomas

alecthomas Feb 6, 2016

Owner

This is the import causing the tests to fail.

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 6, 2016

I originally felt that keeping completions outside of the app was the best idea, as it's more isolated and controlled (plus an easier feature IMO :P ), but, it didn't give users of kingpin great options for handling dynamic arguments without re-generating the script every time.

That's a good point. Looking at the example completion code, it looks like it could be super powerful. Very nice.

I originally attempted this, and also implemented a hybrid, however due to how bash (and ZSH) handle completion selection, a space will always be added (unless there are multiple options to choose from).

That's a bummer, but good to know you had the same thought :)

I think once you've addressed the few minor comments I made, this LGTM.

@nickw444 nickw444 force-pushed the atlassian:nwhyte/autocompletion branch 2 times, most recently from eb85d0e to 09e122d Feb 7, 2016

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 7, 2016

I've made the requested modifications. I removed that args argument for HintAction since it was not used (in fact it was just left over from early development on this feature).

However, I still can't get a green build. I've attempted using a relative package import, and not importing at all. What do you suggest I do to remediate this?

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 9, 2016

The tests are failing because you're importing gopkg.in/alecthomas/kingpin.v2 in the example main.go. You need to import github.com/alecthomas/kingpin.

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 9, 2016

Apart from that, looks great, thanks!

Nicholas Whyte

@nickw444 nickw444 force-pushed the atlassian:nwhyte/autocompletion branch from 09e122d to 2ca6c6a Feb 9, 2016

alecthomas added a commit that referenced this pull request Feb 10, 2016

Merge pull request #99 from atlassian/nwhyte/autocompletion
Add Bash Completion Support.

Many thanks to @nickw444 for doing this!

@alecthomas alecthomas merged commit a833a4c into alecthomas:master Feb 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 10, 2016

This is brilliant, thanks so much @nickw444. Much appreciated.

@ches

This comment has been minimized.

ches commented Feb 11, 2016

Very nice, thank you @nickw444!

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 16, 2016

Hey @alecthomas just wondering when you'll be building a release with these changes for gopkg.in? Cheers

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 17, 2016

I just pushed out v2.1.11

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 17, 2016

Thanks!

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 19, 2016

@nickw444 I've noticed that the command completion in particular seems inconsistent. _examples/curl doesn't seem to work at all. Any idea why? I haven't had time to look at it.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 19, 2016

I'll take a quick look now.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 19, 2016

I think I may have accidentally by chance found the issue you were having:

  1. Build _examples/curl.
  2. Run ./curl and perform tab completion.
./curl <TAB>
file:      ftp://     gopher://  http://    https://

The shell will respond with completion that doesn't seem consistent with kingpin completion. This is because we didn't source the kingpin completion script.

Run eval "$(./curl --completion-script-zsh)". Try again:

./curl --<TAB>
--headers  --help     --timeout  --version

./curl <TAB>
(no output)
@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 19, 2016

I didn't see the first output, I saw no output, as you do with your last line. However, the curl example has commands and subcommands, which don't get completed. I'd expect:

./curl <TAB>
help get post
@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 19, 2016

I can reproduce this issue now. I'm sure this is because of the pattern being used to create this command line tool. Notice how the commands are instantiated from the kingpin library, rather than from an app struct:

...
get         = kingpin.Command("get", "GET a resource.").Default()
getFlag     = get.Flag("test", "Test flag").Bool()
getURL      = get.Command("url", "Retrieve a URL.").Default()
...

All the flags work because they're defined on the command.

Look at the completion example and how the commands are defined:

func addSubCommand(app *kingpin.Application, name string, description string) {
    c := app.Command(name, description).Action(func(c *kingpin.ParseContext) error {
        fmt.Printf("Would have run command %s.\n", name)
        return nil
    })
    c.Flag("nop-flag", "Example of a flag with no options").Bool()
}
...
func main() {
    ...
    addSubCommand(app, "nmap", "Additional top level command to show command completion")
    ...
}
...
@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 19, 2016

They're functionally equivalent though, so I'm not sure why that would make a difference?

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Feb 19, 2016

You're right about that. I'll take a look on Monday. Good find!

@alecthomas

This comment has been minimized.

Owner

alecthomas commented Feb 19, 2016

👍

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