shellcheck: Disable SC2207, as it messes with COMPREPLY

[NoahGorny]

Disable SC2207

Motivation and Context

This shellcheck option is really messing our code, as we use COMPREPLY in many places

How Has This Been Tested?

Tested locally, saw that no such warnings were emitted the change

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[NoahGorny]

@cornfeedhobo @nwinkler @davidpfarrell can you give me a hand here?

[cornfeedhobo]

@NoahGorny no problem. It's a busy weekend but I'll do my best. Can you explain what you were doing to stumble on this so I can reproduce your thinking and/or issue?

[NoahGorny]

@NoahGorny no problem. It's a busy weekend but I'll do my best. Can you explain what you were doing to stumble on this so I can reproduce your thinking and/or issue?

Alright. So what happens is that when we autocomplete something, lets say Bash-it itself, we usually do this in this kind of pattern:

_bash-it-comp-enable-disable()
{
  local enable_disable_args="alias completion plugin"
  COMPREPLY=( $(compgen -W "${enable_disable_args}" -- ${cur}) )
}

However, shellcheck is complaining about the way we read compgen output, and prefers us to use read -a. see this for reference.

After some trials and errors by me, I ultimately gave up in the approach of making read -a work. I am unfamiliar with this command and the fact that we use this COMPREPLY=( $(compgen) ) pattern everywhere caused my to think that we should just ignore this warning entirely.

Lemme know what you think about it, but please feel no rush to answer!
Enjoy your weekend
Noah 😄

[BarbUk]

Using brainy theme to understand why we have this issue, we can see the output of compgen with cat -A

compgen -W "${segments}" -- "${cur}" | cat -A

Which output

battery$
clock$
exitcode$
python$
ruby$
scm$
sudo$
todo$

So compgen outputs multiple lines and not one line with multiple words.

The solution is to use a while loop, like explained in SC2207 wiki.

while IFS='' read -r line; do COMPREPLY+=("$line"); done < <(compgen -W "${segments}" -- "${cur}")

I'll update #1790 to reflect that.
Maybe this PR is not needed anymore.

[BarbUk]

@NoahGorny let me know if using a while loop is ok for completion reply.
If you still want to disable warning for SC2207, I'll rollback the changes in #1790

[NoahGorny]

I guess that if this works- we should use it and not ignore the warning.
Thanks for the detailed explanation @BarbUk!
I will close this PR now..

[davidpfarrell]

Hi Team,

Here's what I learned during my work on lint_clean_files which does this:

mapfile -t FILES < <(
	cat clean_files.txt \
		| grep -v -E '^\s*$' \
		| grep -v -E '^\s*#' \
		| xargs -n1 -I{} find "{}" -type f
)

read -a is meant to split works on a single line.

mapfile builds an array from multi-line input.

I suspect we can use something like this:

mapfile -t COMPREPLY < <(
	compgen -W "${enable_disable_args}" -- ${cur}
)

NOTE: This is untested.

If that doesn't work as-is, lemme know and I'll play around with it and figure out the syntax.

[BarbUk]

@davidpfarrell mapfile is available for bash 4.x+ only.
Bash-it is available for Bash 3.2+ as far as I know.

So using a while loop keep this compatibility with bash 3.2+

[cornfeedhobo]

@NoahGorny Okay, I see what you mean. Here are my $0.02:

shellcheck is worried about command output splitting:

Prefer mapfile or read -a to split command output (or quote to avoid splitting)

In the example you provided you aren't splitting a command's output and we know the string, so I consider the current form preferable. Instead of making this disable global though, you should just put it above the command or at the top of the file.

[cornfeedhobo]

@NoahGorny Did you get a chance to read this? I'd much rather keep with the current form than change these lines to use read.

[BarbUk]

@cornfeedhobo

In the example you provided you aren't splitting a command's output and we know the string, so I consider the current form preferable. Instead of making this disable global though, you should just put it above the command or at the top of the file.

+1

Compgen generate a list of matches from a word list, so if we have a clean word list, disabling this warning for the specific line is the best way.

I will update #1790 with a disable.

# shellcheck disable=SC2207
COMPREPLY=($(compgen -W "${actions}" -- "${cur}"))