Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up bash idioms, use modern interpolation style #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

obscurerichard
Copy link

This PR seeks to clean up the bash idioms used in the alias script. It offers some additional help for debugging, it is possible to run this as DEBUG=true bash ./alias to get a line by line rendition of what it is trying to do, and if there are any errors.

It introduces some comments to the alias script that explain its usage, and links back to the documentation.

I have a mind to also make this shellcheck clean but wanted some feedback about whether PRs are welcome in this project before doing further work.

This script should be robust in the face of adding additional error checking, such as the Bash unofficial strict mode.

Copy link
Owner

@rolfschr rolfschr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. LGTM. Feel free to add further improvements (shellcheck). I can also merge this right away if you want.


# When debugging use bash unofficial strict mode
# See http://redsymbol.net/articles/unofficial-bash-strict-mode/
$DEBUG && set -euo pipefail
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always set euo pipefail? (i.e. not only when in debug mode?)

@@ -1,9 +1,25 @@
#!/bin/bash
#!/usr/bin/env bash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rolfschr
Copy link
Owner

rolfschr commented Oct 3, 2023

Oh, and please also feel free to add a line here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants