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

Use compsys completion system for zsh #1569

Merged
merged 7 commits into from
May 3, 2024

Conversation

Farid-NL
Copy link
Contributor

@Farid-NL Farid-NL commented May 1, 2024

Completions in zsh are done via compctl.
This PR update the completion file to use the 'new' completion system: compsys.

The changeset doesn't include descriptions for commands, it just use the current logic implemented for compctl and use compsys instead.

Farid-NL and others added 3 commits May 1, 2024 11:33
The `_rbenv` script will be autoloaded by zsh as long as it's found in $FPATH.
It should be the package manager's responsibility to symlink or move this file
into an appropriate location.
@mislav
Copy link
Member

mislav commented May 2, 2024

Thanks! I have pushed some changes:

  • the script is now renamed to _rbenv to enable autoloading
  • rbenv command completion now includes command descriptions

To try this out, please ensure that the project's completions/ directory is in your zsh's $fpath before it initializes the completion system. After this change ships, I imagine that it should be the responsibility of a package manager to put the _rbenv file into an appropriate location.

Any feedback welcome!

@Farid-NL
Copy link
Contributor Author

Farid-NL commented May 2, 2024

I've pushed a little change.

  • compedef directive takes its arguments as commands to which it will complete, whence the change from _rbenv to rbenv.

It wasn't working for me until that. But it works great!

@Farid-NL
Copy link
Contributor Author

Farid-NL commented May 2, 2024

I imagine that it should be the responsibility of a package manager to put the _rbenv file into an appropriate location.

I would imagine the same as well, however, what about people using the basic git checkout installation method?

I suppose it could be indicated in the README that it is necessary to add project's completions/ in the $fpath. Or tweak rbenv-init, to add the directory to $fpath only in zsh shells, as the commit above.

@mislav
Copy link
Member

mislav commented May 3, 2024

suppose it could be indicated in the README that it is necessary to add project's completions/ in the $fpath.

That's a very good point. Users of “basic git checkout” installation method will have to tweak their zsh environment to get completions working again. This might be slightly disruptive, but I think proper autoloading is a better solution going forward. We can document this in the README and in future release notes.

Or tweak rbenv-init, to add the directory to $fpath only in zsh shells, as the commit above.

That's a solid idea, but changing $fpath has no effect if the completion system has already been initialized. That means that zsh completions for rbenv would either work or not work depending on where eval … rbenv init line is in zsh startup files relative to completion initialization. Since this is a bit tricky, I would rather opt for users (or package managers) managing fpath themselves and not trying to interfere with that process from within rbenv. With that in mind, I might ship this changeset without your last commit. Would that be okay?

@Farid-NL
Copy link
Contributor Author

Farid-NL commented May 3, 2024

Since this is a bit tricky, I would rather opt for users (or package managers) managing fpath themselves and not trying to interfere with that process from within rbenv. With that in mind, I might ship this changeset without your last commit. Would that be okay?

By all means. You're completely right, it might be tricky changing $fpath just like that.

@mislav mislav merged commit a3b98a4 into rbenv:master May 3, 2024
4 checks passed
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.

None yet

2 participants