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

shellenv: Add zsh site-functions to fpath #18524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HeroCC
Copy link

@HeroCC HeroCC commented Oct 7, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR adds the brew zsh site-functions directory to the fpath during brew shellenv, allowing us to enable completions in one line instead of the seven currently suggested on the Shell Completions page.

The shell-completions documentations page states we don't do this by default because managing completions is complex. This is true, however, I still think it's fair to pick the low-hanging-fruit on behalf of the user; many installations already call compinit on their behalf, and by including the homebrew fpath we can give those users completions for free. Remaining users only need to call compinit (assuming they have shellenv already setup).

If this gets accepted, I can make another PR to update the docs, suggesting users eval the shellenv command instead of the manual fpath manipulation instructions.

For the implementation itself, I made a few design choices for backwards compatibility (documented in the commit description), but I'm flexible if someone prefers it another way.

I considered doing a fallthough in the case-statement, but that doesn't work on bash leq 4 (notably, those versions shipped with MacOS by default).

I chose to prepend the value to the array to mirror the homebrew completion [instructions page](https://docs.brew.sh/Shell-Completion).

I also chose to leave off the `-d`irectory check -- zsh seems to tolerate invalid paths, so there isn't much harm in adding it anyway.

I'm flexible on any of these choices should someone feel strongly. However, I imagine this is the best combination given the trade-offs we have.
@HeroCC HeroCC marked this pull request as ready for review October 7, 2024 20:21
@MikeMcQuaid
Copy link
Member

I'm not sure this should be enabled by default. I have seen non-zero numbers of Homebrew packages with issues and people who wish to not have default ZSH behaviours overridden like this.

@HeroCC
Copy link
Author

HeroCC commented Oct 9, 2024

Hmm, maybe we could append to fpath instead of prepend to give priority to other functions? I see fpath as serving a very similar function to PATH / MANPATH -- when you install packages to brew, I'd expect all of it's resources to be available. So I found myself surprised to find this not installed by default, like all of the other *PATHs.

I have seen non-zero numbers of Homebrew packages with issues

Are these issues with completions, or autoloaded functions in general? Any functions / completions installed here would still need to be actively autoloaded before ZSH adopts them, so I don't think this would be a significant behavior change for users that don't already opt-in to completions or autoload their functions.

Skimming through a search of homebrew-core it seems like the vast majority of references to site-functions is for completions anyway.

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.

2 participants