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

Another approach for zsh completion #1218

Closed
wants to merge 2 commits into from
Closed

Another approach for zsh completion #1218

wants to merge 2 commits into from

Conversation

z0rc
Copy link
Contributor

@z0rc z0rc commented Jan 14, 2021

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

I want to open discussion how this project provides completion for zsh. Current situation isn't right and promotes cargo-cult approach to enable zsh completions.

Completion file shouldn't be sourced. It should provide only completion code (source of _command function) for command. Ideally this file should be just template/example, where developer should replace $PROG with name of binary and distribute it with _command name somewhere under project's contrib folder (alongside with man pages, vim syntax etc). As go build system isn't able to properly install this kind of stuff, it's up to package manager (homebrew, dpkg etc) to pick up this _command file and put it under correct $fpath where zsh can autoload it.

Currently this PR changes only zsh completion example. The documentation needs to be updated too. I can look into if there is agreement with current proposal.

Special notes for your reviewer:

There is fundamental problem with this project calling "completions" as "auto-completions", which are different terms. See https://www.gnu.org/software/bash/manual/html_node/Programmable-Completion.html and http://zsh.sourceforge.net/Doc/Release/Completion-System.html for correct terminology. You might want to fix this in future.

@z0rc z0rc requested a review from a team as a code owner January 14, 2021 19:45
@z0rc z0rc requested review from saschagrunert and coilysiren and removed request for a team January 14, 2021 19:45
Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pull in the default branch to fix the failing test please 🙏🏼

@coilysiren
Copy link
Member

@z0rc Can you pull in the default branch to fix the failing test please 🙏🏼

Completion file shouldn't be sourced. It should provide only completion
code (source of _command) for command. It's task for package manager or
user to put under $fpath.
@z0rc z0rc marked this pull request as draft January 29, 2021 12:51
@z0rc
Copy link
Contributor Author

z0rc commented Jan 29, 2021

@lynncyrin done.

This is work in progress, still requires consensus about proposal and documentation updates if proposal accepted. Marked as draft and awaiting your input.

There is no need to define custom shell var, when Zsh can be detected by
checking SHELL env var.
@z0rc
Copy link
Contributor Author

z0rc commented Jan 29, 2021

Also refactored how zsh shell is detected. $SHELL shows login shell which is almost always user's current shell, unless they do something weird like launching zsh from bashrc.

@z0rc z0rc requested a review from coilysiren March 18, 2021 15:08
@coilysiren coilysiren requested review from a team and removed request for saschagrunert and coilysiren April 23, 2021 19:44
@stale
Copy link

stale bot commented Jul 22, 2021

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale stale due to the age of it's last update label Jul 22, 2021
@stale
Copy link

stale bot commented Aug 22, 2021

Closing this as it has become stale.

@stale stale bot closed this Aug 22, 2021
@z0rc z0rc deleted the better-zsh-completion branch August 22, 2021 22:34
@meatballhat meatballhat removed the status/stale stale due to the age of it's last update label Apr 22, 2022
meatballhat added a commit that referenced this pull request May 22, 2022
Another approach for zsh completion (#1218)
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.

3 participants