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

refactoring #46

Merged
merged 1 commit into from
Dec 7, 2017
Merged

refactoring #46

merged 1 commit into from
Dec 7, 2017

Conversation

davidchambers
Copy link
Owner

Commit message:

This commit is intended to improve readability by:

  • introducing an abort function for writing an error message to standard error then exiting with exit code 1;

  • storing options in an associative array rather than in separate variables; and

  • validating command-line arguments against the aforementioned associative array (to avoid enumerating all the options).

This is the last invasive change I will make before releasing v3.0.0. :)

Copy link
Collaborator

@Avaq Avaq left a comment

Choose a reason for hiding this comment

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

Looks good! Just a question:

xyz
--script)
scripts+=("$1") ; shift ;;
--dry-run|--edit)
options[${option:2}]=true ;;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't find the ${parameter:value}-syntax in this page. Only parameter:[-=+?]value. What does this do?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is substring extraction (i.e. string slicing). See ${string:position} and ${string:position:length}. In this case we're just stripping the leading hyphens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh! :)

xyz
options[${BASH_REMATCH[1]}]="$1" ; shift
else
abort "Unrecognized option $option"
fi
Copy link
Owner Author

Choose a reason for hiding this comment

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

-n ${options[${BASH_REMATCH[1]}]+x} requires an explanation.

BASH_REMATCH:

An array variable whose members are assigned by the ‘=~’ binary operator to the [[ conditional command (see Conditional Constructs). The element with index 0 is the portion of the string matching the entire regular expression. The element with index n is the portion of the string matching the nth parenthesized subexpression. This variable is read-only.

So, if option is --branch, ${BASH_REMATCH[1]} will be branch.

We then wish to determine whether branch is a key of options. The naive approach is to test the result of the lookup with -n, but this assumes the null string will never be used as a value. The thing we actually want to determine is whether the key exists. For this we can use parameter expansion:

declare -A arr=([foo]=FOO [bar]='')

echo "foo: ${arr[foo]+x}" # foo: x
echo "bar: ${arr[bar]+x}" # bar: x
echo "baz: ${arr[baz]+x}" # baz:

So, ${options[${BASH_REMATCH[1]}]+x} will evaluate to x if the specified option exists; the null string otherwise. We then use -n to differentiate these two cases.

This commit is intended to improve readability by:

  - introducing an abort function for writing an error message to
    standard error then exiting with exit code 1;

  - storing options in an associative array rather than in separate
    variables; and

  - validating command-line arguments against the aforementioned
    associative array (to avoid enumerating all the options).
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.

None yet

2 participants