meuleman / epilogos

Methods for summarizing and visualizing multi-biosample functional genomic annotations
https://epilogos.net
GNU General Public License v3.0
41 stars 5 forks source link

General bash suggestions #16

Closed alexpreynolds closed 3 years ago

alexpreynolds commented 3 years ago

The following script may not be obsolete:

https://github.com/meuleman/epilogos/blob/80bf1f5b0846b495d40eda0cd0315c8c0d371b2c/scripts/preprocess_data_ChromHMM.sh#L1-L33

Some general suggestions that I have are:

  1. Use set -ueo pipefail below the shebang line to ensure variables are set and to handle error states more closely. You could even add an -x to log each command. This can be useful for pipelines, log statements, or debugging a problematic script.

  2. Try to avoid using uppercase variable names in bash scripts. Some variables in bash are reserved POSIX names and your intended values will get clobbered, if you accidently use these variable names. It can lead to non-obvious and frustrating errors.

  3. Use curly braces around positional variables like $1 to ${1}, $2 to ${2} etc. Not a big deal, here, but a good habit to get into as this allows you to specify more than nine input parameters ($10 will not work, but ${10} will, for example).

alexpreynolds commented 3 years ago
  1. I might also suggest using double-quotation marks around variables that are at risk of containing space characters. Using double-quotes will preserve spaces, but without them, a filename could become two or more filenames.

Lines 20-22 could be vulnerable to that, if the BIOSAMPLE name from the first column of METADATA might contain sample names with any spaces. Though less risky, GENOME and CHR could also be vulnerable, if they are provided with spaces, for whatever reason (user error, etc.).

jacobquon commented 3 years ago

@meuleman Did you update this script? Or should I do so

meuleman commented 3 years ago

I will work on it

On Mon, Mar 29, 2021 at 11:27 AM Jacob Quon @.***> wrote:

@meuleman https://github.com/meuleman Did you update this script? Or should I do so

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/meuleman/epilogos/issues/16#issuecomment-809610352, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMKAJPWXYIDPD6IM7DN7DTGDBCBANCNFSM4Z2WHBBA .

meuleman commented 3 years ago

Refactored the code and added concrete example data and command, here: https://github.com/meuleman/epilogos/commit/50bbbf82711489c5abd9c07b96bfe0ca98ee0f18