search

dsifford / yarn-completion

Bash completion for Yarn
MIT License
277 stars 25 forks source link

Fix: double-quoted operand #33

Closed takuyahara closed 5 years ago

takuyahara commented 5 years ago

Non-double-quoted operand at line 402 caused error on Bash 5.0.3 / macOS 10.14.3.

bash: /usr/local/Cellar/git/2.21.0/etc/bash_completion.d/yarn-completion.bash: line 402: syntax error in conditional expression: unexpected token `('
bash: /usr/local/Cellar/git/2.21.0/etc/bash_completion.d/yarn-completion.bash: line 402: syntax error near `@(g'
bash: /usr/local/Cellar/git/2.21.0/etc/bash_completion.d/yarn-completion.bash: line 402: `                                      if [[ $prev == @(get|delete) ]]; then'
dsifford commented 5 years ago

Are you certain this is an issue with the completions or could this be an issue with your system?

Double quotes shouldn't be needed in double brackets because bash doesn't word split in double brackets.

takuyahara commented 5 years ago

I quickly investigated a detail.

Firstly the issue has happened on my system because extglob was off and when I turned it on issue has solved. I guess you are turning it on. Double-quoting just looked like solved because it turned if-block syntactically correct, but useless as it should be every time false. You are right, double-quoting wasn't a solution.

Secondary availability of extglob depends on system and you should know that as I see L985-L988 and L1165. The same thing may have to be coded in every function that uses extglob, and also at the top to turn it on and at the bottom to revert. Or instructing user to run shopt -s extglob can also be a solution.

dsifford commented 5 years ago

The same thing may have to be coded in every function that uses extglob, and also at the top to turn it on and at the bottom to revert.

The way it's written should carry the shell option through each function because the execution environment is in the same shell (i.e., no subshell environments). So it's strange that you've observed this behavior.

I'll keep this open for now until I get more people running into it and decide from there. For now, you're the first ever that I'm aware of.

takuyahara commented 5 years ago

The way it's written should carry the shell option through each function because the execution environment is in the same shell (i.e., no subshell environments).

Okay, so shopt may not necessary to be coded in each function.

As far as I tested extglob had to be turned on at the top. In my understanding the reason is that every lines are parsed when the script has loaded but not when ran.

dsifford commented 5 years ago

Yes, adding shopt to the top of the script would indeed work, but it would set the option globally which is not ideal. It needs to take place in the main entrypoint of the completion script and then reset before exiting (as it is now).

Not sure why this isn't working for your shell specifically, because it should be doing that. And by all measures it is doing that on all my machines.

stefankolb commented 5 years ago

I'm having the exact same issue:

-bash: /Users/stefan/.dotfiles/bash/autocomplete/autocomplete-yarn.bash: line 410: syntax error in conditional expression: unexpected token `('
-bash: /Users/stefan/.dotfiles/bash/autocomplete/autocomplete-yarn.bash: line 410: syntax error near `@(g'
-bash: /Users/stefan/.dotfiles/bash/autocomplete/autocomplete-yarn.bash: line 410: `                    if [[ $prev == @(get|delete) ]]; then'

Bash version: 3.2.57(1)-release (x86_64-apple-darwin18) MacOS: 10.14.5

dsifford commented 5 years ago

Bash 3 is not supported as it is now 10 years outdated.