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

caveats: Differentiate zsh completion files and function files #1616

Merged
merged 2 commits into from Dec 12, 2016

Conversation

@zachwhaley
Copy link
Contributor

zachwhaley commented Dec 4, 2016

  • 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 tests with your changes locally?

When installing a file to zsh/site-functions directory, it is assumed this is a zsh completion file,
and the zsh completion caveat is printed after installation.

But not all files in the zsh/site-functions directory are completion files.
Some are files for functions that can be loaded on demand with zsh's autoload command.

  • Edit Keg.completion_installed to search specifically for files in the zsh/site-functions
    directory starting with an underscore only (By convention, zsh completion files start with an underscore)
  • Add Keg.zsh_functions_installed to search for non-completion files in the zsh/site-functions
  • Add Caveats.zsh_functions_caveats to print a caveat if non-completion files have been installed
    to zsh/site-functions
dir = path.join("etc", "bash_completion.d")
when :zsh
# By convention, zsh completion files start with an underscore
return !Dir["#{path}/share/zsh/site-functions/_*"].empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 4, 2016 Member

This early return inside a case statement is a bit weird. Could you rework it so it shares the logic below?

This comment has been minimized.

@zachwhaley

zachwhaley Dec 4, 2016 Author Contributor

The zsh case is looking for specific files in a directory, while the other shell cases are just looking for a non-empty directory.

I'm not sure how I might rework this so they all use the logic below. Do you have a suggestion?

def zsh_functions_installed?
# Check for non completion functions (i.e. files not started with an underscore),
# since those can be checked separately
!Dir["#{path}/share/zsh/site-functions/*"].reject{ |f| f[0] == "_" }.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 4, 2016 Member

Use .start_with? here.

This comment has been minimized.

@zachwhaley

zachwhaley Dec 4, 2016 Author Contributor

Thanks!
Do y'all want me to amend this commit with changes or create new commits?

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 4, 2016

What software installs ZSH functions and what are they used for? Thanks!

@zachwhaley
Copy link
Contributor Author

zachwhaley commented Dec 4, 2016

@MikeMcQuaid Zsh plugins and frameworks use zsh functions, and they are basically command line commands for zsh.

My little project uses a zsh function to override and enhance a command.

@zachwhaley zachwhaley force-pushed the zachwhaley:zsh_functions_caveats branch from 9aa0fa5 to 583ed30 Dec 4, 2016
@zachwhaley
Copy link
Contributor Author

zachwhaley commented Dec 4, 2016

Fair warning, I can't directly test these changes since I don't have a Mac with me; sorry :(

@@ -297,12 +297,18 @@ def lock
def completion_installed?(shell)
dir = case shell
when :bash then path.join("etc", "bash_completion.d")
when :zsh then path.join("share", "zsh", "site-functions")
when :zsh then path.join("share", "zsh", "site-functions") if path.join("share", "zsh", "site-functions").children.any? { |f| f.basename.to_s.start_with?("_") }.empty?

This comment has been minimized.

@zachwhaley

zachwhaley Dec 4, 2016 Author Contributor

I feel like there should be a cleaner way to do this.
Any Ruby devs have a suggestion?

This comment has been minimized.

@zachwhaley

zachwhaley Dec 4, 2016 Author Contributor

Oops, I don't think the empty? at the end was necessary.

@zachwhaley zachwhaley force-pushed the zachwhaley:zsh_functions_caveats branch 2 times, most recently from 7fdf612 to 22f6c4a Dec 4, 2016
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 7, 2016

@MikeMcQuaid Zsh plugins and frameworks use zsh functions, and they are basically command line commands for zsh.

My little project uses a zsh function to override and enhance a command.

My little project uses a Fish function to override and enhance a command.

@zachwhaley To make a core change we'll probably want a few example projects rather than just your personal one. Could you link to a few? Thanks!

@zachwhaley
Copy link
Contributor Author

zachwhaley commented Dec 8, 2016

Honestly, I don't think I'll be able to find a lot of examples for this, outside of zsh specific programs.

This PR and PR #1615 were created from issue #1594 and the discussion there.
This PR is addressing issue #1594, and I added PR #1615 from @apjanke comments about advising the user that something went to a directory that may not be on their path
For PR #1615 that path would be the users $fish_function_path

Since I thought these changes wouldn't be very big, I thought I might as well give it a try :)

Thanks!

@zachwhaley zachwhaley force-pushed the zachwhaley:zsh_functions_caveats branch from 22f6c4a to f79a692 Dec 8, 2016
@zachwhaley zachwhaley mentioned this pull request Dec 8, 2016
3 of 5 tasks complete
@zachwhaley
Copy link
Contributor Author

zachwhaley commented Dec 8, 2016

I did manage to find some packages that could utilize this.

Here are a list of packages that install a zsh script to some location on the system, and then ask the user to source that file, when instead they could install those scripts to zsh/site-functions and ask the user to run an autoload on their script, like what is done in my project. I haven't verified this to be the case for every package, but it seems sensible.

(I was honestly surprise to find this many packages that weren't using zsh's autoload btw 😄)

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 8, 2016

Perfect, thanks for explaining so well. Happy to merge this when conflicts with your other (merged) PR are resolved and would ❤️ PRs for those formulae to use this after that.

zachwhaley added 2 commits Dec 8, 2016
When installing a file to zsh/site-functions directory, it is assumed this is a zsh completion file,
and the zsh completion caveat is printed after installation.

But not all files in the zsh/site-functions directory are completion files.
Some are files for functions that can be loaded on demand with zsh's autoload command.

- Edit Keg.completion_installed to search specifically for files in the zsh/site-functions
  directory starting with an underscore only (By convention, zsh completion files start with an underscore)
- Add Keg.zsh_functions_installed to search for non-completion files in the zsh/site-functions
- Add Caveats.zsh_function_caveats to print a caveat if non-completion files have been installed
  to zsh/site-functions
@zachwhaley zachwhaley force-pushed the zachwhaley:zsh_functions_caveats branch from f79a692 to 14f4662 Dec 8, 2016
@MikeMcQuaid MikeMcQuaid merged commit c317c3c into Homebrew:master Dec 12, 2016
3 checks passed
3 checks passed
codecov/patch 83.33% of diff hit (target 63.10%)
Details
codecov/project 63.11% (+0.01%) compared to 276d009
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 12, 2016

Thanks again for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@zachwhaley zachwhaley deleted the zachwhaley:zsh_functions_caveats branch Dec 12, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.