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

Zsh Completion #646

Merged
merged 28 commits into from
Jun 7, 2019
Merged

Zsh Completion #646

merged 28 commits into from
Jun 7, 2019

Conversation

babysnakes
Copy link
Contributor

@babysnakes babysnakes commented Mar 2, 2018

I improved the existing zsh completion with support for flags and some of the custom completion commands from bash completion. A description of what's supported could be found i the zsh completion readme. Thanks to everyone in the #107 thread for their help, especially to @eparis for explaining things to me over and over again ...

One important note is that I only tested it on a small project on my laptop. It needs to be tested on a larger project and more important on other people's shell configurations to make sure it works correctly.

A very basic POC. Need to refactor to generate completion
structure before passing to the template to avoid repeated
computations.

What works:
  * Real zsh completion (not built on bash)
  * Basic flags (with long flag and optional shorthand)
  * Basic filename completion indication (not with file extensions though)

What's missing:
  * File extensions to filename completions
  * Positional args
  * Do we require handling only short flags?
cmd.Use is not the command name :). Found it once I figured out
that I need to execute the command in order to fully test the
generated completion.
Also make the template more elegant on the way...
- removed redundant function
- improved other functions :)
- better names for other functions
- If the flags are not bool the completion expects argument.
- You don't have to specify file extensions for file completion to
  work.
- Allow multiple occurrences of flag if type is stringArray.

Need to verify that these assumption are correct :)
They were committed by mistake.
Fixed after input from @eparis:
- Decide on option parameter by checking NoOptDefVal
- Slices also could be specified multiple times.
Yet another temporary file that found itself in the repo :(
I thought there was a bug in the boolSlice definition but it seems
It was my mistake in identifying what's going on. Also removed the
provisioning to skip tests (doesn't seem to be needed anymore).
@babysnakes
Copy link
Contributor Author

I encountered an issue (with a utility that I write) that affect completion. Not sure if/how to solve this issue. I have a Version configured on my root command so I'll have --version flag. To generate the completion I created a completion sub-command. The symptom was that when I ran <mycmd> completion zsh --file ... It didn't show completion for --version. This is because InitDefaultVersionFlag operates on the executed command and not on the root command and since my completion command doesn't have Version it doesn't generate it.

Should I at least point it out in the zsh completion readme?

@eparis
Copy link
Collaborator

eparis commented Mar 4, 2018

Does that mean that --help doesn't show up either? Do we want to make those late flag init calls a function, which we explicitly call from the generator command tree walk code?

@babysnakes
Copy link
Contributor Author

You're right about the --help, I was destructed by the fact that the help subcommand did show up but yeah, InitDefaultHelpCmd runs on the root command while InitDefaultHelpFlag and InitDefaultVersionFlag runs on the actual command you run. If there's a good reason it behaves that way and we don't want to change it then we can extract it to a separate function call.

@babysnakes
Copy link
Contributor Author

@eparis, it seems that the whole --version and --help thing is my error. I need a little time to do some more checks but I believe it's my fault 😞 . Should fix in a short while.

It was running on the command it was invoked from which caused some
additional helpers (--help, --version) not to be generated.
@babysnakes
Copy link
Contributor Author

There's something really strange here. On my tests (last commit: 70f3e35) it behaves correctly but on my real command that I'm using it doesn't. damn.

When invoking from subcommand. Modified the test to prove.
@babysnakes
Copy link
Contributor Author

Ok, sadly the problem still exist. The problem was in my test. Added a skipped test to show real issue. Back to square one in terms of dealing with --version and --help. @eparis, sorry for spamming.

@breunigs
Copy link

I tried your patch for rclone, and it seems much more complete! I believe single quotes ' in the descriptions are not escaped, which leads to misleading error messages (/usr/share/zsh/vendor-completions/_rclone:1987: unmatched " – the line doesn't have " and the actual broken single quote was somewhere around line 5000). Anyway, after manually replacing all occurrences of single quotes in rclone's descriptions, the completion works.

It seems that all subcommands (e.g. rclone copy source:path dest:path [flags]) are missing the file completion option. So, the --some-flag will autocomplete, but it will not suggest local files. For the bash completion this seems to work. I still have to checkout if this is maybe setup incorrectly in the command description on rclone's side.

@babysnakes
Copy link
Contributor Author

Thanks for the input @breunigs, Do you have your changes committed in a branch or should I just try modifying master? I'll try to check it later today.

@breunigs
Copy link

breunigs commented Mar 17, 2018 via email

@joshuarubin
Copy link

She passed away last week

ברוך דיין האמת

@MichaelMure
Copy link
Contributor

@babysnakes as much as I would like to see your work land in Cobra, please take all the time you need. Don't feel obligated to work on it if you don't feel like it.

lukehoban pushed a commit to pulumi/pulumi that referenced this pull request Sep 24, 2018
…#1967)

## Why ?

I'm using Zsh (and I'm not the only one 🤣). Pulumi having Zsh completions is great. I will also add completions to the Homebrew Formula when this is merged.

## Why not use Cobra `GenZshCompletion`

It's currently not good enough. Maybe it will be when spf13/cobra#646 is done.

## Implementation

I did the same thing `kubectl` does for Zsh completion. Meaning using the bash completion generated by Cobra and adapting it to a zsh format. The resulting zsh completion file is not perfect (compared to one's where you have a short command description in the output) but it's good enough I think. 

I also changed the file output to a stdout output. I think it's better than outputting to a file and it will make adding completions in Homebrew straightforward. I don't know if the previous `gen-bash-completion` is used in any Pulumi project so this may break things.
@WGH-
Copy link

WGH- commented Nov 2, 2018

Thank you for your work. Even though it's not merged yet, your code is already incredibly useful for generating completion definitions, storing them as an asset, and amending them manually.

I noticed one small issue so far. When usage string contains multiple lines, descriptions looks very broken, even though --help itself looks fine and properly indented. I think the solution is to only include the first line of the usage in the description.

@rsteube
Copy link
Contributor

rsteube commented Nov 28, 2018

Awesome work.
I needed custom completion for positional arguments and persistent flags so i added that in a fork.
If it's any help you can see my changes here: babysnakes/cobra@zsh-completion...rsteube:zsh-completion-custom

@hoshsadiq
Copy link

Any updates on this?

@babysnakes
Copy link
Contributor Author

Hi @hoshsadiq and everyone else.

Sorry for disappearing but I can't seem to find enough time to get into bash-completion (and I can't merge the completion of zsh and bash without it). I'm pretty fluent with zsh completion, but I tried several times already to understand how bash-completion works and I didn't manage to figure it out in the very limited time I had. The good news is that I found a team member who is ready to help so I hope to sync him on the status and hopefully he'll manage to take it further.

@juliangilbey
Copy link

There's also a forked version of this at github.com/rstuebe/cobra which is now being used by github.com/zaquestion/lab. So it would be good to do a fair bit of merging when it's feasible...
Thanks!

@rsteube
Copy link
Contributor

rsteube commented May 16, 2019

Can't we get this merged somehow? I don't really understand whats the hold-up with bash completion since this targets the zsh completion and i don't expect it to solve everything at once.
As a user i'm already very happy with what is generated - be it the bash or zsh completion (apart from the small fixes i did).

I would rather like to see this merged in the current state and fixes be done in succeeding smaller MR's than having it lying around another year.
I can try to help if there is something to do.

@rsteube
Copy link
Contributor

rsteube commented Jun 4, 2019

Maybe a temporary alternative until the PR is merged: https://github.com/rsteube/cobra-zsh-gen

I extracted the zsh_completions.go changes and added a simple wrapper struct.
This way it can be invoked as an additional dependency without changes to the cobra codebase.

import zsh "github.com/rsteube/cobra-zsh-gen
zsh.Wrap(RootCmd).GenZshCompletion(os.Stdout)

example: zaquestion/lab@master...rsteube:use-cobra-zsh-gen

It's a bit crude but seems to work so far.

@pior
Copy link

pior commented Jun 4, 2019

Thanks @rsteube

Maybe this issue will get a bit more visibility with macOS switching to ZSH

@spf13
Copy link
Owner

spf13 commented Jun 7, 2019

@babysnakes @eparis Thanks for this great contribution. Sorry I sat on things for such a long time. I'm trying to be a better steward. Merging 👍

@spf13 spf13 merged commit e2c45ac into spf13:master Jun 7, 2019
@babysnakes
Copy link
Contributor Author

Hey @spf13,

I wasn't available for a few days. Thank you for merging it. Sadly, I'm not sure it was fully ready yet. I phased out of it when I realised I couldn't get any support with merging bash completion with zsh completion. I tried to find someone from work to help but it didn't work as well. In any case, I'm still ready to do the work on the zsh end if someone is ready to help me on the bash completion end.

Thanks

@corneliusweig
Copy link

Can you remind me why this affects bash completion? If you are aiming for a unified completion code, that's very much worth a separate PR IMHO. As a first step, we should get zsh completion right.

@babysnakes What in your opinion is missing to make it fully ready?

@babysnakes
Copy link
Contributor Author

@corneliusweig, my original idea was to get something working (that was many months ago). One of the first feedbacks I got (which I completely agree with) is that we better merge the completions so users will not have to work hard on configuring bash and zsh separately. I actually did the first steps (on my laptop, never committed it - check the long thread to see why) but really got stuck at trying to figure out bash completion.

The problem with merging this pull request before merging the completions is that now we have to worry about 3 APIs to be backward compatible (bash, zsh and unified).

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

Successfully merging this pull request may close these issues.