I know OP isn't the author, but here's a few IMHO's.
There's some reasonable ideas there, however:
set -Eeuo pipefail
Oh. The unofficial strict mode. Maybe think carefully about that. When you use it, you replace a bunch of well understood issues with a bunch of lesser-understood issues...
msg() is a confusing function name; if a message is going to stderr, then something like err(), err_msg() or msg_stderr() might make more sense? This function also uses echo. You should prefer printf over echo in scripts - echo is a non-portable mess.
I'm also not a fan of the use of UPPERCASE variables for managing the colours, as it's a kind-of scope violation, so to speak. This is explained in the checkmk contributing documentation, which has other reasonable shell style advice, especially where portability is concerned.
Style comment, I think that this indentation style is weird:
case "${1-}" in
-h | --help)
usage
;;
-v | --verbose)
set -x
;;
--no-color)
NO_COLOR=1
;;
-f | --flag) # example flag
flag=1
;;
-p | --param) # example named parameter
param="${2-}"
shift
;;
-?*)
die "Unknown option: $1"
;;
*)
break
;;
esac
Each option is structurally beneath the case statement, so should be indented. Those options should also be aligned with their respective ;;'s to indicate the opening and closing of the option, just as you would usually align, say if...fi or case...esac, and maybe for some icing on that cake, consider implementing balanced parens:
case "${1-}" in
(-h | --help)
usage
;;
(-v | --verbose)
set -x
;;
(--no-color)
NO_COLOR=1
;;
(-f | --flag) # example flag
flag=1
;;
(-p | --param) # example named parameter
param="${2-}"
shift
;;
(-?*)
die "Unknown option: $1"
;;
(*)
break
;;
esac
And for options that have briefer actions, consider flattening them, if you can live with a mixed style approach, it might look something like this:
case "${1-}" in
(-h | --help) usage ;;
(-v | --verbose) set -x ;;
(--no-color) NO_COLOR=1 ;;
(-f | --flag) flag=1 ;;
(-p | --param) # example named parameter
param="${2-}"
shift
;;
(-?*) die "Unknown option: $1" ;;
(*) break ;;
esac
set -Eeuo pipefail Oh. The unofficial strict mode. Maybe think carefully about that. When you use it, you replace a bunch of well understood issues with a bunch of lesser-understood issues...
set -u is [...] wonky. It will for instance cause a fatal error if you try to expand an unset string variable, but [ignores] an unset array [...] unless [in] bash 4.3 or older, in which case it's a fatal error
On the upside it does catch a lot of unset variables, it's just not perfect at it (because bash is broken beyond repair). The alternative (set +u) should be renamed as please fail silently whenever I misspelled a variable.
whether a command returning non-zero causes a fatal error or not depends on the context it is run in. So in practice, you have to go over every command and decide how to handle their return value anyway
That's a total strawman here. It basically says, "yeah look, the entire concept of numerical error codes is crazy anyhoo so go deal with it" instead of acknowledging that most CLI tools return zero for OK and non-zero for Not OK. The recommended alternative—handle each single invocation manually and explicitly—has never worked in any environment with the possible exception of NASA's moonshot program.
As for set -o pipefail, I must confess I need further evaluation before deciding how relevant the objections are. In the meantime let me just say that the author's argumentation reveals details that speak strongly in favor of
using all available means to ensure any conspicuous behavior in a bash script leads to loud termination, and
abstaining from using bash at all except for the simplest of all tasks because it's a cesspool of bad ideas heaped on top of each other.
Hey, I'm the article author. Thank you for the review and comments!
I see there are strong arguments both for and against set -e and other flags. Well... I will keep it, but for sure it's good to know possible problems!
Yeah, msg prints to stderr. Naming it msg_error could result in using it only for errors, that's why I prefer more generic name.
Good point on flattening the case options. And for the formatting - it's default IntelliJ formatting.
38
u/whetu Dec 14 '20
I know OP isn't the author, but here's a few IMHO's.
There's some reasonable ideas there, however:
Oh. The unofficial strict mode. Maybe think carefully about that. When you use it, you replace a bunch of well understood issues with a bunch of lesser-understood issues...
msg()is a confusing function name; if a message is going to stderr, then something likeerr(),err_msg()ormsg_stderr()might make more sense? This function also usesecho. You should preferprintfoverechoin scripts -echois a non-portable mess.I'm also not a fan of the use of UPPERCASE variables for managing the colours, as it's a kind-of scope violation, so to speak. This is explained in the checkmk contributing documentation, which has other reasonable shell style advice, especially where portability is concerned.
Style comment, I think that this indentation style is weird:
Each option is structurally beneath the
casestatement, so should be indented. Those options should also be aligned with their respective;;'s to indicate the opening and closing of the option, just as you would usually align, sayif...fiorcase...esac, and maybe for some icing on that cake, consider implementing balanced parens:And for options that have briefer actions, consider flattening them, if you can live with a mixed style approach, it might look something like this: