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.

Sign up for GitHub

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

Jump to bottom

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

Open

benhancock wants to merge 2 commits into netlify:main from benhancock:oct-16

+139 −3

Contributor

benhancock commented Oct 17, 2024 •

edited

Loading

🎉 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:

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

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:

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;
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:

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

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 and others added 2 commits

October 16, 2024 17:30


          fix: fixed bugs in completion

7cbc687

Co-authored-by: Dylan Spyer <[email protected]>


          fix: add zsh autocompletion setup and file permissions instructions t…

f0bc35d

…o completion:install

Co-authored-by: Dylan Spyer <[email protected]>

benhancock requested a review from a team as a code owner

October 17, 2024 15:55

Contributor

dylanspyer commented Oct 17, 2024

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.

benhancock mentioned this pull request

Completions don't work #6724

Open

benhancock commented

View reviewed changes

src/commands/completion/completion.ts

+                    ])
+                    .then((answer) => {
+                      if (answer['compinitAdded']) {
+                        fs.readFile(zshConfigFilepath, 'utf8', (err, data) => {

Contributor Author

benhancock Oct 17, 2024 •

edited

Loading

Note that we add the autoload -U compinit; compinit line to the top of the ~/.zshrc file (and not the bottom or right before the tabtab config line) because

it needs to be run before the tabtab config line ('[[ -f ~/.config/tabtab/__tabtab.zsh ]] && . ~/.config/tabtab/__tabtab.zsh || true') and
because compinit is also needed for other completions in general (besides netlify/tabtab) to work with zsh

benhancock commented

View reviewed changes

src/commands/completion/completion.ts

+                log(`Completion for ${parent.name()} successfully installed!`)
+                if (process.platform !== 'win32') {
+                  log("\nTo ensure proper functionality, you'll need to set appropriate file permissions.")

Contributor Author

benhancock Oct 17, 2024

Because chmod is a Unix command, we only output this instruction if the user is not on windows

DanielSLew requested changes

View reviewed changes

Contributor

DanielSLew left a comment

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

src/commands/completion/completion.ts

Comment on lines +31 to +32

		const TABTAB_CONFIG_LINE = '[[ -f ~/.config/tabtab/__tabtab.zsh ]] && . ~/.config/tabtab/__tabtab.zsh \|\| true'
		const AUTOLOAD_COMPINIT = 'autoload -U compinit; compinit'

Contributor

DanielSLew Oct 19, 2024

Throw these in the constants file in this folder

src/commands/completion/completion.ts

Comment on lines +42 to +51

+                  await inquirer
+                    .prompt([
+                      {
+                        type: 'confirm',
+                        name: 'compinitAdded',
+                        message: `Would you like to add it?`,
+                        default: true,
+                      },
+                    ])
+                    .then((answer) => {

Contributor

DanielSLew Oct 19, 2024

Generally you wouldn't use await and .then() you would use one or the other. If you want it to be blocking use await, if you want it to not be blocking use .then()

Suggested change

      
                await inquirer
          
                  .prompt([
          
                    {
          
                      type: 'confirm',
          
                      name: 'compinitAdded',
          
                      message: `Would you like to add it?`,
          
                      default: true,
          
                    },
          
                  ])
          
                  .then((answer) => {
          
                const answer = await inquirer
          
                  .prompt([
          
                    {
          
                      type: 'confirm',
          
                      name: 'compinitAdded',
          
                      message: `Would you like to add it?`,
          
                      default: true,
          
                    },
          
                  ])

src/commands/completion/completion.ts

+                    ])
+                    .then((answer) => {
+                      if (answer['compinitAdded']) {
+                        fs.readFile(zshConfigFilepath, 'utf8', (err, data) => {

Contributor

DanielSLew Oct 19, 2024

I would await this. So await fs.readFile or use fs.readFileSync.

src/utils/command-helpers.ts

+                try {
+                  filecontent = fs.readFileSync(filename, 'utf8')
+                } catch (error_) {
+                  // @ts-expect-error TS(2571) FIXME: Object is of type 'unknown'.

Contributor

DanielSLew Oct 19, 2024

Pull in the main branch to get updates from this PR #6877 to fix this type error

tests/integration/commands/completion/completion-install.test.ts

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'

Contributor

DanielSLew Oct 19, 2024

pull these from constants when you make the change from above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet