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

Load asdf completions #8837

Closed
wants to merge 2 commits into from

Conversation

filipegiusti
Copy link

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.

Other comments:

May fix asdf-vm/asdf#692

It's a short term solution for #8779

Fix #8820

Tagging listed Maintainer of OMZ asdf plugin: @RobLoach

@ohmyzsh ohmyzsh bot added the Area: plugin Issue or PR related to a plugin label Apr 16, 2020
@filipegiusti
Copy link
Author

@jthegedus may also be interested.

@jthegedus
Copy link

Thanks @filipegiusti I am indeed interested.

Comment on lines 15 to 16
fpath=($ASDF_COMPLETIONS $fpath)
autoload -Uz compinit && compinit
Copy link

@jthegedus jthegedus Apr 17, 2020

Choose a reason for hiding this comment

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

This shouldn't be performed for any brew installed version of asdf as Homebrew has it's own setup for completions, which I believe I implemented in https://github.com/Homebrew/homebrew-core/blob/6e728e63ab37243b6832d2995e7efdbac7f0d612/Formula/asdf.rb#L25 (the correctness of my implementation needs to be validated).

Copy link

@jthegedus jthegedus Apr 17, 2020

Choose a reason for hiding this comment

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

I believe #L8 can be removed also as the Homebrew install should now handle the completions.

#L8 needs to be modified to conform to https://docs.brew.sh/Shell-Completion#configuring-completions-in-zsh

Copy link
Author

Choose a reason for hiding this comment

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

I'm not familiar with brew, so I may be completely wrong here.

The documentation suggest that FPATH=$(brew --prefix)/share/zsh/site-functions:$FPATH is needed to install all zsh completions when using brew, not just one plugin. ZSH brew plugin also doesn't do anything related to completions.

I think that your changes on brew formulae for asdf should sync link the _asdf file and as users of brew follow the instructions on https://docs.brew.sh/Shell-Completion#configuring-completions-in-zsh, it just work ®.

Copy link
Author

Choose a reason for hiding this comment

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

I made a few changes to not add the completion code on brew, however I can't test it on brew.

Copy link
Author

Choose a reason for hiding this comment

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

I have a suspicion that we should also be handling here different versions of asdf. Which asdf version added the zsh completion?

Choose a reason for hiding this comment

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

I can perform a test of brew tomorrow, though am also not particularly familiar.

0.7.6 was the last version this plugin properly supports.

0.7.8 added the ZSH completions properly.

Copy link

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

This looks good to me for supporting asfd 0.7.8 and above. If backwards compatibility is required, then asdf 0.7.6 was the last version that relied upon the Bash completions

@mcornella
Copy link
Member

Running compinit again in the plugin will cause duplicate work and slow down the startup significantly. My suggestion in #8779 (comment) was to change $fpath, then autoload the completion and compdef, something like this:

fpath=("$ASDF_DIR/completions" $fpath)
autoload -Uz _asdf
compdef _asdf asdf

That said, if the bash completion is present in all present asdf versions, the proper solution is to use it in conjunction with bashcompinit, which is already loaded by OMZ, so asdf-vm/asdf#692 shouldn't be an issue here.

I'm closing this then, in wait of new information.

@mcornella mcornella closed this May 5, 2020
@jthegedus
Copy link

jthegedus commented May 6, 2020

@mcornella our goal with this plugin would be to use the ZSH completions and not rely on the bash completions. The confusion in that linked issue is people expecting this to load the ZSH completions with compinit, but getting the bash completions and not having the deps of our bash completions script.

Without the call to compinit in this plugin, would this work without users having to call compinit after they initialise OMZ and it's plugins? My understanding was compinit ran before any plugins and to get the effect we want we would need to run it again in the plugin here.

@Quintasan
Copy link

Soooooo this was closed and the only solution for now is to either source asdf completions BEFORE o-m-z without using the plugin or using the bash completions?

@mcornella
Copy link
Member

As I said, the bash completion (which is perfectly functional) is present in all versions, unlike the zsh one. So it makes sense to keep it that way.

@jthegedus
Copy link

@Quintasan That's exactly what I do https://github.com/jthegedus/dotfiles/blob/c58a76734caa2d7bbba6acdeadcc439326dde0bb/dotfiles/.zshrc#L71

I agree not ideal.

@mcornella I feel

bash completion is present in all versions, unlike the zsh one

Is not a sound reason to avoid solving for ZSH native completions. The fact the Bash works and the compinit issues are a cause for headaches is a fair point though.

I hope there's a solution to this in the future, but until then I agree with @mcornella in using the Bash completions if you use this plugin.

@digitalmoksha
Copy link

Argh! This should work out of the box, I've already wasted an hour trying to get this to work

@gaving
Copy link

gaving commented Mar 4, 2021

Came here from https://asdf-vm.com/#/core-manage-asdf

Confused 😵

I now have:-

autoload -U +X bashcompinit && bashcompinit
plugins=(
  asdf
  ....

To get things working.

@TheTechRobo
Copy link
Contributor

TheTechRobo commented Mar 4, 2021

@gaving The way I understand this PR is, you have to specifically load the PR in omz. Its not included by default.

Try omz pr test 8837 and see how that works. :nod:

@jthegedus
Copy link

jthegedus commented Mar 4, 2021

@gaving what I suggest here - #8837 (comment) - is this:

fpath=($HOME/.asdf/completions $fpath)
plugins=(
	asdf
	...
)

The reason the documentation does not just say "Do X" is because there are many different ways to manage the ZSH completions depending on Shell manager (if one is used at all). You shouldn't need to run autoload etc with this OMZSH plugin as long as you include the Shell completions with fpath before the OMZSH plugins are listed.

Perhaps we should list each shell manager's instructions in the docs, though as solution in the plugin would be ideal, perhaps this PR can be modified to perform these suggestions #8837 (comment)

@gaving
Copy link

gaving commented Mar 4, 2021 via email

@fxsalazar
Copy link

fxsalazar commented Mar 25, 2021

I tried exactly #8837 (comment)

fpath=(${ASDF_DIR}/completions $fpath)
plugins=(
  asdf
)

source $ZSH/oh-my-zsh.sh

and still got the error .asdf/completions/asdf.bash:68: command not found: complete even when deleting the .zcompdump file. and restarting the terminal. Only solution was adding the autoload/compinit lines and removing the plugin.

Version:

asdf --version
v0.8.0-c6145d0

kennethkalmer added a commit to kennethkalmer/dotfiles that referenced this pull request Mar 25, 2021
Use my own asdf loading snippet to work around ohmyzsh/ohmyzsh#8837
@jthegedus
Copy link

@fxsalazar Check if ASFD_DIR is populated at that point in your shell setup, I would wager it is not set and therefore the completions dir not being found properly

@fxsalazar
Copy link

fpath=(${ASDF_DIR}/completions $fpath)
plugins=(
  asdf
)
>> la .asdf/completions
total 40
-rwxr-xr-x  1  staff   6.2K Mar 25 12:42 _asdf
-rw-r--r--  1  staff   2.0K Mar 25 12:42 asdf.bash
-rw-r--r--  1  staff   6.9K Mar 25 12:42 asdf.fish
------------------------------------------------------------

Same result.

@jthegedus
Copy link

fpath=(${ASDF_DIR}/completions $fpath)
plugins=(
  asdf
)
>> la .asdf/completions
total 40
-rwxr-xr-x  1  staff   6.2K Mar 25 12:42 _asdf
-rw-r--r--  1  staff   2.0K Mar 25 12:42 asdf.bash
-rw-r--r--  1  staff   6.9K Mar 25 12:42 asdf.fish
------------------------------------------------------------

Same result.

What did you do differntly? If ASDF_DIR is not set before you use it in the fpath code then what you are doing is not going to work.

Also, you said

I tried exactly #8837 (comment)

But fpath=(${ASDF_DIR}/completions $fpath) != fpath=($HOME/.asdf/completions $fpath). Please try the exact code with $HOME and not $ASDF_DIR and report your result.

@fxsalazar
Copy link

fxsalazar commented Apr 3, 2021

fpath=($HOME/.asdf/completions $fpath)
plugins=(  
  asdf
)

source $ZSH/oh-my-zsh.sh

# . $HOME/.asdf/asdf.sh
# fpath=(${ASDF_DIR}/completions $fpath)
# autoload -Uz compinit && compinit

==>

# init terminal
Last login: Sat Apr  3 15:01:41 on ttys005
~/.asdf/completions/asdf.bash:68: command not found: complete

# asdf [tab]
~ » asdf _asdf:60: command not found: compgen   

I did re-launch the terminal; cleaned the .zcomp* files.

loganlinn added a commit to loganlinn/dotfiles that referenced this pull request Sep 23, 2021
@devinrhode2
Copy link

@jthegedus do you know if the docs on asdf-vm.com are outdated or not?
CleanShot 2022-12-20 at 15 34 10

CleanShot 2022-12-20 at 15 35 20

@jthegedus
Copy link

@devinrhode2 The docs were written when this PR was pending, not closed. I am not a ZSH user so not sure if the issue has been resolved elsewhere or not. Happy to update the docs should that caveat no longer be the case.

@devinrhode2
Copy link

Seems like it's still buggy in zgen

@GraemeF
Copy link

GraemeF commented Feb 2, 2023

The completion is working fine for me (oh-my-zsh plugin on macos)

@owpac
Copy link
Contributor

owpac commented Mar 26, 2023

Hey guys!

The completions for ZSH is not yet supported by asdf plugin. I opened a PR here 6 months ago to resolve this. TBH, I'm not sure to perfectly understand all the issues around the ZSH completions problems. Is my PR resolving the issues? Is my PR not relevant?

@iloveitaly
Copy link

This is fixed in the asdf + direnv plugin redxtech/zsh-asdf-direnv#10

@asadakbar
Copy link

@carlosala Has this issue been corrected by your changes here 278bcfc? asdf is still referencing this issue in its zsh and git guide https://asdf-vm.com/guide/getting-started.html#_3-install-asdf.

@carlosala
Copy link
Member

It should work, yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: plugin Issue or PR related to a plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$HOME/.asdf/completions/asdf.bash:68: command not found: complete