Error and initial check
I recommend to substitute $1
with $@
, so that if you ever forget to quote it will still print the complete message.
err ()
{
echo "redo: error: $@"
echo "redo: usage: redo <migration_name>"
exit 1
}
I am not sure why you put the usage in that function, since if there is no database, this does not help anyone.
[[ -d ./db/migrate ]] || err "dir db/migrate not found"
You also may want to check if you have too few/many arguments.
[[ -z $1 ]] && err No argument!
[[ $# -gt 1 ]] && err Two many arguments!
Finding the migrations
I recommend not using sed
to truncate your pathnames. I also think it is completely superfluous since your trapping a few things later on and can simply ignore the prefix. You also do not need to use grep
, find
should be able to do that on his own. Hence instead of
migrations=$(find './db/migrate' -name '*.rb' | grep "$1" | sed 's#^\./db/migrate/##')
simply use
migrations=$(find './db/migrate' -name "*$1*.rb")
At this point I would check if it actually found anything, too.
[[ -z $migrations ]] && err I don't know what you are talking about.
Note that I omitted the quotes to make the point raised above.
Creating the array
I recommend not using times
as a variable name, because there is a built-in bash command of the same name.
You can use avoid that echo |sed
construct by using the build-in regular expressions comparison operator =~
.
Since I don't know the exact form how the migrations are written, I can only assume that a migration comes in the form ./db/migrate/YYYYMMDDHHmmSS_name.rb
. Where the timestamp is obviously just numbers, but name can be anything.
declare -A migration_times
for select_migration in $migrations; do
pattern="([0-9]+)_([^\.]+)\.rb"
if [[ $select_migration =~ $pattern ]]; then
migration_times["${BASH_REMATCH[2]}"]="${BASH_REMATCH[1]}"
fi
done
That shortens your code quite considerably and should make it more effective since you are not calling another program.
Executing command or listing migrations
If the array does only have one element then you don't need to read the whole pair, you can simply use the value the complete array evaluates to, saving you the lines with reassigning the names. You can also skip the exit 0
statement because that would overwrite the exit status of your external call. You can tie the listing with an else
statement to the conditional.
I also used printf
to give it a nicer look if the length of the names vary %-10s
has to be adjusted to your needs.
if [[ ${#migration_times[@]} == 1 ]]; then
echo "redo: executing 'bundle exec rake db:migrate:redo VERSION=${migration_times[*]}'"
# bundle exec rake db:migrate:redo VERSION=${migration_times[*]}
else
echo "redo: be more specific, candidates are:"
for element in "${!migration_times[@]}"; do
printf "%-10s : %s\n" $element ${migration_times[$element]}
done
exit 1
fi
I think you can also easily migrate the changes from your answer to this review.