Add check for autoloading compinit in zsh.go #124

ratticon · 2020-06-27T23:25:39Z

Encountered issue when trying to install autocompletion for terraform on fresh MacOS with default zsh configuration, using their terraform -install-autocomplete command.
Raised issue on terraform git and found it imports this module.
Cloned zsh.go and added check for the line that autoloads the zsh compinit.
Please review, thanks!

Encountered issue when trying to install autocompletion for terraform on fresh MacOS with default zsh configuration, using their `terraform -install-autocomplete` command. Raised issue on terraform git (hashicorp/terraform#25421) and found it imports this module. Cloned zsh.go and added check for the line that autoloads the zsh compinit. Please review, thanks!

codecov · 2020-06-27T23:27:28Z

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   82.43%   82.43%           
=======================================
  Files          10       10           
  Lines         541      541           
=======================================
  Hits          446      446           
  Misses         75       75           
  Partials       20       20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c05e97...fcba7c8. Read the comment docs.

ratticon

Manually adding the autoload command to ~/.zshrc was recommended by @alisdair and is consistent with the zsh documentation: http://zsh.sourceforge.net/Doc/Release/Completion-System.html#Use-of-compinit

An alternative solution would be to check for the line and echo a prompt for the user to run the compinstall command.
compinstall guides the user through setting up zsh autocompletion but required reading and selecting various options.
After answering all compinstall prompts with defaults, the following lines are added to ~/.zshrc:

# The following lines were added by compinstall
zstyle :compinstall filename '/Users/username/.zshrc`

autoload -Uz compinit
compinit
# End of lines added by compinstall

posener

Thanks for the CL!
One small note - regarding the context of the CL, I'm not sure that transformer are using complete/v2 (https://github.com/hashicorp/terraform/blob/master/go.mod#L114) which the master is pointing on.
If you want it to work with terraform, we should cherry-pick it to v1 (branch v1) and create a new release - then update it in terraform's go.mod.

posener · 2020-06-29T20:57:42Z

install/zsh.go

@@ -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) {


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?

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

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

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"

@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.

Fixed bug in compInit check that would duplicate bashcompinit line in if both bashcompinit and compinit lines are missing from z.rc file.

memark · 2021-11-26T20:05:34Z

@ratticon @posener What's the status on this? Still running into this issue. Today it was the autocomplete for the latest version of Terraform that didn't work at all on a fresh Mac.

ratticon · 2021-12-11T01:30:05Z

@ratticon @posener What's the status on this? Still running into this issue. Today it was the autocomplete for the latest version of Terraform that didn't work at all on a fresh Mac.

Hi @memark!

I'm sorry for letting this fall to the wayside, I don't currently have access to a new Mac to figure this out. Hashicorp closed the issue I raised: terraform -install-autocomplete not working on stock macOS Catalina 10.15.5 with default zsh shell on Jul 28, 2020.

Does adding:

autoload -U +X bashcompinit && bashcompinit
autoload -Uz compinit && compinit

to ~/.zshrc and running exec zsh or restarting zsh enable you to run terraform -install-autocomplete without error?

Kind Regards,
Ben

ratticon mentioned this pull request Jun 27, 2020

terraform -install-autocomplete not working on stock macOS Catalina 10.15.5 with default zsh shell hashicorp/terraform#25421

Closed

ratticon commented Jun 28, 2020

View reviewed changes

posener reviewed Jun 29, 2020

View reviewed changes

ratticon marked this pull request as draft June 30, 2020 03:22

Update zsh.go

fcba7c8

Fixed bug in compInit check that would duplicate bashcompinit line in if both bashcompinit and compinit lines are missing from z.rc file.

ratticon marked this pull request as ready for review December 11, 2021 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for autoloading compinit in zsh.go #124

Add check for autoloading compinit in zsh.go #124

ratticon commented Jun 27, 2020

codecov bot commented Jun 27, 2020 •

edited

Loading

ratticon left a comment •

edited

Loading

posener left a comment

posener Jun 29, 2020

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

memark commented Nov 26, 2021

ratticon commented Dec 11, 2021

Add check for autoloading compinit in zsh.go #124

Are you sure you want to change the base?

Add check for autoloading compinit in zsh.go #124

Conversation

ratticon commented Jun 27, 2020

codecov bot commented Jun 27, 2020 • edited Loading

Codecov Report

ratticon left a comment • edited Loading

Choose a reason for hiding this comment

posener left a comment

Choose a reason for hiding this comment

posener Jun 29, 2020

Choose a reason for hiding this comment

ratticon Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

ratticon Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

ratticon Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

ratticon Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

memark commented Nov 26, 2021

ratticon commented Dec 11, 2021

codecov bot commented Jun 27, 2020 •

edited

Loading

ratticon left a comment •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading

ratticon Jun 30, 2020 •

edited

Loading