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

fix: add zsh autocomplete setup and file permissions instructions to completion:install #6882

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

benhancock
Copy link
Contributor

@benhancock benhancock commented Oct 17, 2024

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes Completions don't work

There are at least two issues that might cause completions to fail after running completion:install:

  1. On Linux/MacOS, the completion script may not have the necessary executable permissions.
  • Fix: Run chmod +x with the file path to the completion script
  1. Netlify uses Tabtab for completions, and with zsh, Tabtab (and other) completions do not work until compinit has been run.
  • Fix: Add the line autoload -U compinit; compinit to the user’s ~/.zshrc file above the Tabtab config line to load and then run compinit

This PR addresses both of these issues by:

  1. automatically outputting the chmod +x command with the appropriate file path for the user to copy and run if needed after the installation is complete;
  2. prompting the user to have the autoload -U compinit; compinit line added to the top of their .zshrc file automatically if not already present.

Here's what the netlify completion:install output looks like before:

Screenshot 2024-10-17 at 11 44 44 AM

Here's what the netlify completion:install output looks like after our changes:

Screenshot 2024-10-17 at 11 45 41 AM

This PR also creates a new test file for completion:install with tests for the compinit -> ~/.zshrc functionality.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@benhancock benhancock requested a review from a team as a code owner October 17, 2024 15:55
@dylanspyer
Copy link
Contributor

We decided not to modify file permissions on the user's behalf and instead output the chmod command for the user to run manually. This ensures that the user maintains ownership of their security and configuration settings.

Copy link
Contributor

@DanielSLew DanielSLew left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few changes that I've requested

src/commands/completion/completion.ts Outdated Show resolved Hide resolved
src/commands/completion/completion.ts Outdated Show resolved Hide resolved
src/commands/completion/completion.ts Outdated Show resolved Hide resolved
src/utils/command-helpers.ts Outdated Show resolved Hide resolved
@benhancock benhancock requested a review from a team as a code owner October 25, 2024 18:10
Comment on lines +10 to +11
const TABTAB_CONFIG_LINE = '[[ -f ~/.config/tabtab/__tabtab.zsh ]] && . ~/.config/tabtab/__tabtab.zsh || true'
const AUTOLOAD_COMPINIT = 'autoload -U compinit; compinit'
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the exports you created in the other file already

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.

Completions don't work
3 participants