search

mbucc / shmig

Database migration tool written in BASH.
BSD 3-Clause "New" or "Revised" License
461 stars 50 forks source link

Reconsider fixing shellcheck SC2155 #61

Closed shamrin closed 5 years ago

shamrin commented 5 years ago

Shellcheck SC2155 says "declare and assign separately to avoid masking return values". For example, it prefers:

local migration
migration=$(...)

versus original:

local migration=$(find_migrations ...)

I think it's not a good idea to follow SC2155 in shmig:

  1. We never need to check for return codes in these local declarations.
  2. It becomes easy to forget proper local declaration when it's on a separate line. We even have an example in the latest commit to master: https://github.com/mbucc/shmig/commit/2d2ccc52a0b8ff58b8f17d30ec3dd73f0528f525#diff-b77c435350fe92985b2413c0bde725feR496. Unfortunately, shmig is not catching this error.
mbucc commented 5 years ago

I took a look at the top-five starred Shell projects in github when searching for POSIX, then looked if they follow SC2155. They agree with you.

project uses local obeys SC2155 notes
nvm.sh Y Y Loses errors b/c of pipes?
acme.sh N n/a Not a single instance of 'local ' in acme.sh. Just uses global namespace; e.g., https://github.com/Neilpang/acme.sh/blob/master/acme.sh#L382-L386
ok.sh Y N Only uses a bare local when the method has branches, and each branch sets the variable to a different value. Taught me the POSIX ${varname?:error message} idiom; this looks useful---for example, local level="${1:?Level is required.}"
modernish N n/a The author looks like a POSIX maven; aliases local to typeset at one point for some reason (compatability I think).
osync.sh Y N Like ok.sh, uses bare local when a method has branches, and each branch sets variable to a different value.

My main reasons for doing it was:

  1. I couldn't tell from inspection if the command substitution $() could raise an error;
  2. Failing fast and hard is the fastest and easiest way to fix bugs.
  3. It is an easy way to make sure we don't ignore this class of errors

I find correct error handling in shell scripting challenging; for example, the default behavior with pipes.

But I do find the code harder to read and agree that most (all?) commands cannot raise an error, so given your preference and the evidence above, I'm fine reverting the change.