Skip to content

Add check for autoloading compinit in zsh.go #124

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add check for autoloading compinit in zsh.go #124

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7a6ebd
Add check for autoloading compinit in zsh.go
ratticon Jun 27, 2020
fcba7c8
Update zsh.go
ratticon Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions install/zsh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "fmt"
// basically adds/remove from .zshrc:
//
// autoload -U +X bashcompinit && bashcompinit"
// autoload -Uz compinit && compinit
// complete -C </path/to/completion/command> <command>
type zsh struct {
rc string
Expand All @@ -23,9 +24,13 @@ func (z zsh) Install(cmd, bin string) error {

completeCmd := z.cmd(cmd, bin)
bashCompInit := "autoload -U +X bashcompinit && bashcompinit"
compInit := "autoload -Uz compinit && compinit"
if !lineInFile(z.rc, bashCompInit) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not this is 100% correct.
Does the order of the commands important?
If the file already have the completeCmd line, but doesn't have the compInit in it?

Copy link
Author

@ratticon ratticon Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for looking Eyal.

Let me rubber-duck for a moment:
This line defines compInit as the string to search for / add, that same way the bashCompInit line is handled.
Before the suggested changes, the conditional at line 28 looks for the line in the zsh config file matching the bashCompInit string, then builds the completeCmd string to append to the file.
In my suggested changes, I added the conditional at line 31 to check for the compInit line in the zsh config and add both the bashCompInit and compInit commands plus the completeCmd string that may / may not have already been added to by the previous conditional.

You are correct! This could result in completeCmd string being mutated and appended like so:
(newline chars expanded for easier reading)

25: completeCmd = "example_command"
[true] 29: completeCmd = 
   "autoload -U +X bashcompinit && bashcompinit
   example_command"
[true] 31: completeCmd = 
   "autoload -U +X bashcompinit && bashcompinit
   autoload -Uz compinit && compinit
   example_command
   autoload -U +X bashcompinit && bashcompinit
   example_command"

I'm sorry, I will re-think this.

As for the order of the commands. I haven't tested that.
I will edit my ~/.zshrc file and see if autocomplete works properly with the autoload bashcompinit... and autoload compinit... commands in a different order.

Kind Regards,

Ben

Copy link
Author

@ratticon ratticon Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I tested two versions of my ~/.zshrc file and found that both enable autocompletion without throwing any errors.

Test A:

autoload -Uz compinit && compinit
autoload -U +X bashcompinit && bashcompinit
complete -o nospace -C /usr/local/bin/terraform terraform

Test B:

autoload -U +X bashcompinit && bashcompinit
autoload -Uz compinit && compinit
complete -o nospace -C /usr/local/bin/terraform terraform

Copy link
Author

@ratticon ratticon Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I changed line 32 from:
completeCmd = bashCompInit + "\n" + compInit + "\n" + completeCmd
to
completeCmd = compInit + "\n" + completeCmd
to avoid the issue the bug I identified above.

Test 1 - both bashcompinit and compinit lines missing from z.rc
(newline chars expanded for easier reading)

25: completeCmd = "example_command"
[True] 29: completeCmd = 
   "autoload -U +X bashcompinit && bashcompinit
   example_command"
[True] 32: completeCmd = 
   "autoload -Uz compinit && compinit
   autoload -U +X bashcompinit && bashcompinit
   example_command"
[Result] 35: completeCmd = 
   "autoload -Uz compinit && compinit
   autoload -U +X bashcompinit && bashcompinit
   example_command"

Test 2 - only bashcompinit line missing from z.rc
(newline chars expanded for easier reading)

25: completeCmd = "example_command"
[True] 29: completeCmd = 
   "autoload -U +X bashcompinit && bashcompinit
   example_command"
[False] 32:  Skipped. completeCmd not modified.
[Result] 35: completeCmd = 
   "autoload -U +X bashcompinit && bashcompinit
   example_command"

Test 3 - only compinit line missing from z.rc
(newline chars expanded for easier reading)

25: completeCmd = "example_command"
[False] 29: Skipped. completeCmd not modified.
[True] 32:  completeCmd = 
   "autoload -Uz compinit && compinit
   example_command"
[Result] 35: completeCmd = 
   "autoload -Uz compinit && compinit
   example_command"

Test 4 - both bashcompinit and compinit lines exist in z.rc
(newline chars expanded for easier reading)

25: completeCmd = "example_command"
[False] 29: Skipped. completeCmd not modified.
[False] 32: Skipped. completeCmd not modified.
[Result] 35: completeCmd = "example_command"

Copy link
Author

@ratticon ratticon Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@posener, regarding:

If the file already have the completeCmd line, but doesn't have the compInit in it?

This is also an issue with the existing check for the bashCompInit line, right?

Maybe we should add a dedicated function to check if zsh has been configured and fix it before adding commands, for example:

func (z zsh) Configure() error {
	missingConfigLines := ""
	bashCompInit := "autoload -U +X bashcompinit && bashcompinit"
	compInit := "autoload -Uz compinit && compinit"
	if !lineInFile(z.rc, bashCompInit) {
		missingConfigLines = bashCompInit
	}
	if !lineInFile(z.rc, compInit) {
		missingConfigLines = missingConfigLines + "\n" + compInit
	}
	return appendFile(z.rc, missingConfigLines)
}

Sorry if above example is declared incorrectly. I'm coming from a Python background, but picking up Go while working through this change request with you. I'm not sure if I can declare missingConfigLines as an empty string or if returning appendFile(z.rc, "") would throw an exception.

completeCmd = bashCompInit + "\n" + completeCmd
}
if !lineInFile(z.rc, compInit) {
completeCmd = compInit + "\n" + completeCmd
}

return appendFile(z.rc, completeCmd)
}
Expand Down