Update Styleguide

[bastelfreak]

I would like to propose a change to our styleguide. We currently have a lot of functions with var checking like this:

function foobar {
  if [ -n "$1" ]; then
    # some magic
  fi
}

I would like to move it to a oneliner:

E_NOTENOUGHPARAMS = 4561
function foobar {
  [ -n "$1" ] || return "$E_NOTENOUGHPARAMS"
  # some magic
}

this would save up one level of indentation. @killermoehre or @dhxgit any opinions on this one?

[bastelfreak]

We should keep in mind that this may add a second return statement which is kind of an bash coding antipattern. Nevertheless I would prefer the change.

[killermoehre]

A simple return is very misleading. So you need a message on stderr, too, which gives you again two lines.
Maybe we can abstract this with a function like check_values as first call in every fuction?

[bastelfreak]

we could simply define our own exit code like 5461 which means "insufficient amount of params". I don't see the necessity for a message on stderr

[bastelfreak]

updated the proposed example

[dhxgit]

There is not that much benefit in removing one level of indentation.

But I suggest to go one step further anyways and remove all those checks and commit to run all scripts with set -eu or even set -euo pipefail.

Functions that check for all variables being there, but are called somewhere in the code without all variable should not be possible at all and should be fixed.

[bastelfreak]

that is an even better idea IMO. anybody against/therefor too?

[dhxgit]

-C (noclobber) could also be relevant, it makes it so that > redirection does not override existing files, which could be another safety thing, if you want to overwrite a file, do it explicitly. (>| instead of >)

Would also catch errors where a > should have been a >>.

[dhxgit]

Maybe longopts are better, so that even non-bash-aficionados can deduce what the options are doing.
E.g.:

set -o errexit
set -o noclobber
set -o nounset
set -o pipefail

[bastelfreak]

• I'm not sure about errexit, we've got many places with error handling that would need a rewrite
• noclobber would be bad I think, we use > in many many places, that would require a huge amount of code change. Also > seems to be more common than >|. We've got another open issue, regarding tests. They should cover it when somebody transposed > and >>.
• 👍 for nounset
• 👍 for pipefail