postmodern / ruby-install

Installs Ruby, JRuby, TruffleRuby, or mruby
MIT License
1.89k stars 250 forks source link

Fix Shellcheck violations #443

Open ixti opened 1 year ago

ixti commented 1 year ago

Resolves: #442

postmodern commented 1 year ago

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 commented 1 year ago

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