Define a convention for parsing command-line flags

[jdeanwallace]

Resolves #3

Looking at the shell scripts in our repos, we seem to parse command-line flags in basically four different ways. This PR compares these methods and picks a convention based on our preference.

1. Using getopts

Pros:

• Uses bash built-in getopts command
• Can chain short flag names (e.g. script.sh -abcd)
• Built-in error handling
• Code is compact
• Currently, the majority of our shell scripts parse command-line flags in this way

Cons:

• Only supports short flag names
• Need to learn the options syntax (e.g. getopts 'ht:f')

Example:

TARGET_FILE=''
FORCE='false'
# Parse command-line flags.
# One colon indicates that the previous flag expects a required value, while
# two colons to indicate an optional value.
while getopts 'ht:f' opt; do
  case "${opt}" in
    h)
      echo 'Help is on its way'
      exit
      ;;
    t)
      TARGET_FILE="${OPTARG}"
      ;;
    f)
      FORCE='true'
      ;;
    *)
      >&2 echo 'Sorry, invalid option'
      exit 1
  esac
done
readonly TARGET_FILE
readonly FORCE

2. Using getopt

Pros:

• Supports both short and long flag names
• Can specify flag values using = (e.g. script.sh --target-file=/tmp/file)
• Built-in error handling

Cons:

• Uses GNU based getopt command (i.e., not available on macOS)
• Need to learn the options syntax
• Performs unexpected flag matching
  For example, script.sh -target /tmp/file results in TARGET_FILE=arget.
  This isn't an issue in the other parsing methods.

Example:

# Parse command-line flags.
# One colon indicates that the previous flag expects a required value, while
# two colons to indicate an optional value.
OPTIONS="$(getopt \
  --options 'ht:f' \
  --longoptions 'help,target-file:,force' \
  -- \
  "$@")"

# Process command-line flags.
eval set -- "${OPTIONS}"

TARGET_FILE=''
FORCE='false'
while true; do
  case "$1" in
    -h|--help)
      echo 'Help is on its way'
      exit
      ;;
    -t|--target-file)
      TARGET_FILE="$2"
      shift # For flag name.
      shift # For flag value.
      ;;
    -f|--force)
      FORCE='true'
      shift # For flag name.
      ;;
    --)
      shift
      break
      ;;
    *)
      >&2 echo 'Sorry, invalid option'
      exit 1
  esac
done
readonly TARGET_FILE
readonly FORCE

3. Using a custom implementation for long flags

Pros:

• Code is easier to read and write than getopt / getopts

Cons:

• Completely custom implementation
• No built-in error handling
• Can't specify flag values using =

Example:

TARGET_FILE=''
FORCE='false'
while [[ "$#" -gt 0 ]]; do
  case "$1" in
    --help)
      echo 'Help is on its way'
      exit
      ;;
    --target-file)
      TARGET_FILE="$2"
      shift # For flag name.
      shift # For flag value.
      ;;
    --force)
      FORCE='true'
      shift # For flag name.
      ;;
   *)
      >&2 echo 'Sorry, invalid option'
      exit 1
  esac
done
readonly TARGET_FILE
readonly FORCE

4. Using a custom implementation for short and long flags

Pros:

• Code is easier to read and write than getopt / getopts
• Requires little extra work to support both long and short flag names

Cons:

• Completely custom implementation
• No built-in error handling
• Can't specify flag values using =
• Can't chain short flag names

Example:

TARGET_FILE=''
FORCE='false'
while [[ "$#" -gt 0 ]]; do
  case "$1" in
    -h|--help)
      echo 'Help is on its way'
      exit
      ;;
    -t|--target-file)
      TARGET_FILE="$2"
      shift # For flag name.
      shift # For flag value.
      ;;
    -f|--force)
      FORCE='true'
      shift # For flag name.
      ;;
   *)
      >&2 echo 'Sorry, invalid option'
      exit 1
  esac
done
readonly TARGET_FILE
readonly FORCE

---

Source material

• https://stackoverflow.com/a/402410/3769045
• https://stackoverflow.com/a/34531699/3769045
• https://stackoverflow.com/a/7069755/3769045

[mtlynch]

I think we should go with the GOOD - parses only long flag names option.

The main reason for short flags is to make it easier on users who have to type the command names a lot, but none of our scripts are things that users would regularly type. They're called by other scripts, where we'd want long flags anyway, or they're called by users who are probably copy/pasting an example we gave them.

[cghague]

Apologies for the unprompted comment! I stumbled across this and thought it was worth mentioning that we also use getopt - which is different to getopts - in a few of our scripts, such as the one to set a static IP address for example.

The benefit of getopt is that that it supports both short and long options simultaneously, can enforce mandatory arguments, allows for optional arguments, and can detect invalid options. Our existing scripts that use it are based on the reference implementation and seem easy to understand.

@mtlynch / @jdeanwallace - Would it be worth considering getopt for this convention?

[mtlynch]

@cghague - Good point! We should consider getopt here, as the less we're rolling our own, the better.

[jdeanwallace]

Thanks @cghague, my eyes just glazed over that one.

I've updated the doc with an example using getopt, along with some pros/cons.

For me, this has slightly changed my preferences. If our heart is set on implementing long flag names then I would still stick with using custom implementation #1, but seeing how compact the code for using getopts can be I wouldn't mind using that either.

I'll leave the final decision to @mtlynch (if it's changed since #10 (comment))

[cghague]

Thanks @jdeanwallace. The naming of the two utilities certainly doesn’t help with distinguishing them!

In terms of long flag names, a big bonus of them is they’re less scary to users. It’s a lot more reassuring to run scary-script --fix-issue --reboot --preserve-settings than it is scary-script -frP.

[mtlynch]

@jdeanwallace - Yeah, I'd rather re-use existing tools, but reading the write-up, getopt doesn't seem to save us much code, and the fact that it's not cross platform kind of kills the deal.

Can we move the discussions of pros/cons to the PR description and update the style to just be the convention we're choosing?

Since this is a style PR, it'll go to whole team for review, but I want to have one clear proposal.

[jdeanwallace]

@mtlynch - I've updated the PR description to outline our discussion and reduced the code change to only detail the convention chosen.

[mtlynch]

Automated comment from CodeApprove ➜

---

In: Discussion

3. Using custom implementation A
4. Using custom implementation B

Can we call these, "Using a custom implementation for long flags" and "Using a custom implementation for short and long flags"? It took me a few seconds of comparing the two snippets to spot what the difference was.

---

In: Discussion

Code is easier to read and write

Can we add "easier to read and write than getopt / getopts?

---

In: README.md:

> Line 175
      echo 'Help is on its way'

Can we put in an example of printing usage information?

In practice, we're probably going to come to this page every time we implement a new bash script that takes flags, so it's handy if we can just copy a full, realistic example and replace the parameter names.

We don't have to do a full example in the PR description, just in the README itself.

---

👀 @jdeanwallace it's your turn please take a look

[jdeanwallace]

Automated comment from CodeApprove ➜

---

In: Discussion
Done.

---

In: Discussion
Done.

---

In: README.md:
Done.

---

👀 @mtlynch it's your turn please take a look

[mtlynch]

Automated comment from CodeApprove ➜

✔️ Approved

LGTM!

Can we share it with the team for wider review?

---

👀 @jdeanwallace it's your turn please take a look

[jdeanwallace]

Hey team, when you each get a chance could you review this style proposal for parsing shell command-line flags?

@cghague @jotaen4tinypilot @db39

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ @cghague please review this Pull Request

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ @jotaen4tinypilot please review this Pull Request

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ @db39 please review this Pull Request

[db39]

Automated comment from CodeApprove ➜

✔️ Approved

LGTM!

---

👀 @cghague, @jdeanwallace, @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

✔️ Approved

👍 I also feel this is the best approach for us. (And it’s really a bummer that flag parsing is generally so all over the place in shell/bash…)

---

👀 @cghague, @jdeanwallace it's your turn please take a look

[cghague]

Automated comment from CodeApprove ➜

⏳ Approval Pending (1 unresolved comments)
Approval will be granted automatically when all comments are resolved

LGTM

---

In: README.md:

> Line 198
      >&2 print_help

I don’t think I’ve ever seen redirection to STDERR formatted in this way before. I’ve checked a few of our other scripts at random and we always put the redirection at the end. Should this be changed?

That said, I do like this, it looks so much tidier over multiple lines. If we could get this adopted globally I’d be all for it!

---

👀 @jdeanwallace it's your turn please take a look

[jdeanwallace]

Automated comment from CodeApprove ➜

---

In: README.md:

> Line 198
      >&2 print_help

Yeah I like it too. One other place we use it like this is in bundler/verify-bundle. So I don't think it's wrong, we just haven't defined a convention for it yet...

Until now 🤠 #13

[cghague]

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.