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.

Sign up for GitHub

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

Jump to bottom

zsh: added compdef call to support direct sourcing #887

Closed
wants to merge 1 commit into from
Closed

zsh: added compdef call to support direct sourcing #887

wants to merge 1 commit into from

Conversation

rsteube
Copy link
Contributor

@rsteube rsteube commented Jun 12, 2019

as suggested by @corneliusweig

fixes #881

@rsteube rsteube changed the title added compdef call to support direct sourcing zsh: added compdef call to support direct sourcing Jun 12, 2019
@epk epk mentioned this pull request Jun 17, 2019
Closed
@corneliusweig
Copy link

@rsteube travis has failed. Seems unconnected to me though. Can you rebase/merge and see if the problem persists?

@rsteube
Copy link
Contributor Author

rsteube commented Jun 17, 2019

Seems the shellcheck still fails on master: https://travis-ci.org/spf13/cobra/builds/546829202
(yes it's unrelated to the changes of this branch)

@corneliusweig
Copy link

Are you aware if anybody is taking care of the failing master?

@rsteube
Copy link
Contributor Author

rsteube commented Jun 18, 2019

Haven't seen a related PR/Issue yet.

@rsteube
Copy link
Contributor Author

rsteube commented Jun 19, 2019

#889

@corneliusweig
Copy link

Would be nice to see this land in master 😄

@suleymanakbas91 suleymanakbas91 mentioned this pull request Jul 17, 2019
Closed
@suleymanakbas91
Copy link

We hit the same problem :( Would be great if this gets merged :)

@umarcor
Copy link
Contributor

umarcor commented Jul 18, 2019

We hit the same problem :( Would be great if this gets merged :)

ping @jharshman...

@suleymanakbas91
Copy link

Any idea when this can be merged? @jharshman @corneliusweig @rsteube

@epk
Copy link

epk commented Aug 8, 2019 •
edited

@spf13 Could you get this in? Several projects are blocked on autocompletion issues

@suleymanakbas91
Copy link

@jharshman @spf13 someone, please?

@umarcor umarcor mentioned this pull request Sep 11, 2019
Closed
@rsteube
Copy link
Contributor Author

rsteube commented Sep 26, 2019

Anyone that depends on this can use https://github.com/rsteube/cobra-zsh-gen for now.

@jharshman
Copy link
Collaborator

@rsteube the usage of ZSH completions is documented and follows the pretty standard practice of including the completion script in fpath.

Can you explain the use case for direct sourcing and more importantly; @epk how is this blocking projects?

@epk
Copy link

epk commented Sep 27, 2019

@rsteube the usage of ZSH completions is documented and follows the pretty standard practice of including the completion script in fpath.

Can you explain the use case for direct sourcing and more importantly; @epk how is this blocking projects?

One I can think of right away is
kubernetes-sigs/kind#522

There have see at least 2 other issues where people have tried to do source < (foo completion zsh)

@rsteube
Copy link
Contributor Author

rsteube commented Sep 27, 2019

@jharshman yes, using fpath is the correct and recommended way since it's pre-generated and lazy loaded that way.
It's just that users are accustomed from bash to use source <(command) which is easier, stays up to date but obviously slows down terminal startup time.
So it's rather to prevent issues being opened regarding this problem and since it it doesn't seem to hurt having an explicit compdef call i personally don't mind adding that as additional loading option.

@jharshman jharshman self-requested a review September 30, 2019 15:08
@jharshman
Copy link
Collaborator

@rsteube @epk thank you for explaining the use case as well as your patience during the review process. It is my opinion that this is not a bug, nor an improvement. The usage of ZSH completions is well documented here, and follows the recommended method of sourcing completions and functions outlined in the documentation for ZSH.

@jharshman jharshman closed this Sep 30, 2019
@corneliusweig
Copy link

@jharshman I beg to differ here, I do think this is a bug. Many projects feature a sub-commands like kind completion zsh to generate a zsh completion script [1]. Although it may be recommended practice for zsh to put this script in fpath, this is not how the majority of projects using cobra advocate how to use their completion subcommand. They all say source <( ... ) for both zsh and bash.
I find it difficult to justify to break so many users, just to keep a one-line change out. Or maybe I have not fully grasped the technical implications of this change.
@rsteube can you clarify if this has bad repercussions when used properly in fpath?

[1] Other projects I know which follow the same pattern: minikube, skaffold, kind, kubectl

corneliusweig pushed a commit to corneliusweig/rakkess that referenced this pull request Oct 27, 2019
Add missing compdef call
b485582 
The vanilla version assumes that the completion script is generated once
and then put in the zsh completion script folder. This has advantages,
such as better caching, but it breaks the most common use-case
`source <(rakkess completion zsh)`

This commit adds a compdef call which re-enables this usage.

Also see spf13/cobra#887
corneliusweig pushed a commit to corneliusweig/rakkess that referenced this pull request Oct 28, 2019
@corneliusweig
Add missing compdef call
eb1ac91 
The vanilla version assumes that the completion script is generated once
and then put in the zsh completion script folder. This has advantages,
such as better caching, but it breaks the most common use-case
`source <(rakkess completion zsh)`

This commit adds a compdef call which re-enables this usage.

Also see spf13/cobra#887
@corneliusweig
Copy link

FWIW, the compdef call can be added manually, see https://github.com/corneliusweig/rakkess/blob/eb1ac91e5cddba8f3fbb14d88fff6e863738d8a8/cmd/completion.go#L78 for an example.

corneliusweig added a commit to corneliusweig/skaffold that referenced this pull request Oct 29, 2019
@corneliusweig
Use native zsh completion script generator
eb00885 
So far, Skaffold used a wrapper code borrowed from kubectl to use the bash completion script for zsh. Cobra v0.0.5 introduced a native completion script generator for zsh. To support the `source <(...)` use-case, a minor tweak is necessary (also see spf13/cobra#887)

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig corneliusweig mentioned this pull request Oct 29, 2019
Merged
6 tasks
dgageot pushed a commit to GoogleContainerTools/skaffold that referenced this pull request Nov 6, 2019
@corneliusweig @dgageot
Use native zsh completion script generator (#3137)
7338942 
* Use native zsh completion script generator

So far, Skaffold used a wrapper code borrowed from kubectl to use the bash completion script for zsh. Cobra v0.0.5 introduced a native completion script generator for zsh. To support the `source <(...)` use-case, a minor tweak is necessary (also see spf13/cobra#887)

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>

* Do not explicitly ignore write errors
@srlehn srlehn mentioned this pull request Nov 7, 2019
Closed
@corneliusweig corneliusweig mentioned this pull request Nov 13, 2019
Closed
@shtratos shtratos mentioned this pull request Dec 13, 2019
Open
@jsteenb2 jsteenb2 mentioned this pull request Mar 13, 2020
Merged
4 tasks
@marckhouzam marckhouzam mentioned this pull request Apr 15, 2020
Merged
@marckhouzam marckhouzam mentioned this pull request Feb 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corneliusweig corneliusweig corneliusweig approved these changes

@jharshman jharshman Awaiting requested review from jharshman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New zsh completion script seems broken
6 participants
@rsteube @corneliusweig @suleymanakbas91 @umarcor @epk @jharshman