Fix Shellcheck violations

[ixti]

Resolves: #442

[postmodern]

Spotted a few things. We may want to disable a few more of the ShellCheck rules, since they might not correctly be matching the code.

[postmodern]

Are quotes really needed within bash variable substitutions?

var1="foo bar"
var2="foo bar baz"
echo "${var2#$var1 }"

[ixti]

I don't think so, but shellcheck disagrees :D

[postmodern]

Wait, which version of ShellCheck are you using? I'm on 0.7.2, which might be older than your version, which might explain why I'm not seeing those warnings?

[ixti]

Mine is 0.8.0 :D

[postmodern]

We actually need the implicit shell-splitting here. fetch "$ruby/$file" "$package_manager" will read the dependencies.txt file and return the apt: pkg1 pkg2 ... line that matches the "$package_manager", but without the apt: prefix. It is then implicitly splatted into an Array of individual package names. Reading each line would cause ruby_dependencies to become a singleton Array that contains the whole line (ex: ("pkg1 pkg2 pkg3")).

[ixti]

Hmm, agree. I just followed (blindly) what Shellcheck WIKI was proposing :D

[postmodern]

I'm not a Arch user so I can't verify this, can you double check if pacman -T outputs the missing packages one per-line or a single-line with the package names separated by spaces?
https://archlinux.org/pacman/pacman.8.html

[ixti]

Don't have Arch installed, either. I'm just maintaining Gentoo ebuild in ::guru overlay, and currently I simply disable make check :D

[postmodern]

Wait, I forgot I can check using the archlinux docker image.

[postmodern]

Looking at some of the ShellCheck errors, I think SC2155 might be wrong here:

In share/ruby-install/checksums.sh line 33:
	local output="$(grep "  $file" "$checksums")"
              ^----^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/checksums.sh line 58:
	local output="$($program "$file")"
              ^----^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/checksums.sh line 74:
	local actual_checksum="$(compute_checksum "$algorithm" "$file")"
              ^-------------^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/functions.sh line 22:
	ruby_dependencies=($(fetch "$ruby/$file" "$package_manager" || return $?))
                           ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).


In share/ruby-install/package_manager.sh line 32:
			local brew_owner="$(/usr/bin/stat -f %Su "$(command -v brew)")"
                              ^--------^ SC2155: Declare and assign separately to avoid masking return values.


In share/ruby-install/package_manager.sh line 43:
			local missing_pkgs=($(pacman -T "$@"))
                                            ^---------------^ SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).


In share/ruby-install/ruby-install.sh line 275:
	local fully_qualified_version="$(lookup_ruby_version "$ruby" "$ruby_version")"
              ^---------------------^ SC2155: Declare and assign separately to avoid masking return values.

When you put a command inside of $() the shell will implicitly ignore any returned error codes since it only cares about the output of the command. We might be able to just exclude SC2155 as well. Or we could rewrite all of those spots to use while IFS='' ... so that command being read could fail and also cause the while to fail as well.

[ixti]

I'm fine if those checks will be disabled (globally or locally via comments). Just went the lazy approach by doing what SC proposes and making sure tests pass after that on my machine :D