Add check/all #1146
Add check/all #1146
Conversation
This is adapted from, but heavily modified from, Cirq's check/all.
| If the option --no-coverage is specified, check/pytest will be run instead of | ||
| check/pytest-and-incremental-coverage. |
There was a problem hiding this comment.
I fail to see any code for --no-coverage. Either remove this paragraph or handle the option in the script. Since passing coverage is required by the CI, it seems better to just change the text and keep running pytest-and-incremental-coverage unconditionally.
| thisdir=$(dirname "${BASH_SOURCE[0]:?}") || exit $? | ||
| repo_dir=$(git -C "${thisdir}" rev-parse --show-toplevel) || exit $? | ||
| cd "${repo_dir}" || exit $? |
There was a problem hiding this comment.
Nit - remove || exit $? because it is implied by the errexit (-e) option on line 41.
| declare rev | ||
| declare apply_arg | ||
| declare only_changed |
There was a problem hiding this comment.
Nit - it is safer to set initial value, e.g., declare rev="", otherwise the rev value could be picked from the rev environment variable. Also, as rev and apply_arg are optional for format-incremental, it is better to declare them as arrays which may be empty (and would not trigger shellcheck):
declare -a rev=()
declare -a apply_arg=()
| if [[ -n "${rev}" ]]; then | ||
| error "Invalid arguments." | ||
| echo "${usage}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This silently skips --no-coverage option or any other unknown option if there is no rev argument. How about
| if [[ -n "${rev}" ]]; then | |
| error "Invalid arguments." | |
| echo "${usage}" | |
| exit 1 | |
| fi | |
| error "Invalid option '${arg}'" | |
| error "See '$0 --help' for the list of supported options." | |
| exit 1 |
| if [ "$(git cat-file -t "${arg}" 2> /dev/null)" != "commit" ]; then | ||
| error "No revision '${arg}'" | ||
| exit 1 | ||
| fi | ||
| rev="${arg}" |
There was a problem hiding this comment.
This will reject annotated tag wrapping a commit, because its type is "tag" (this is an issue in Cirq too). Here is a better way:
| if [ "$(git cat-file -t "${arg}" 2> /dev/null)" != "commit" ]; then | |
| error "No revision '${arg}'" | |
| exit 1 | |
| fi | |
| rev="${arg}" | |
| if ! rev=( "$(git rev-parse --verify --end-of-options "${arg}^{commit}")" ); then | |
| error "No revision '${arg}'" | |
| exit 1 | |
| fi |
| declare -a errors | ||
| declare program |
There was a problem hiding this comment.
bash seems to initialize errors[0] from the environment. The program is not used on global scope.
| declare -a errors | |
| declare program | |
| declare -a errors=() |
| local -r program="$1" | ||
| echo "Running $program ..." | ||
| $program || errors+=( "${program} failed" ) | ||
| echo |
There was a problem hiding this comment.
Passing the entire command line as one argument (e.g., line 102) is fragile, in case any of desired arguments contain space or a glob-like character which would get expanded. It is safer to pass the command line arguments exactly:
| local -r program="$1" | |
| echo "Running $program ..." | |
| $program || errors+=( "${program} failed" ) | |
| echo | |
| echo "Running $* ..." | |
| "$@" || errors+=( "$* failed" ) | |
| echo |
| run "check/format-incremental ${rev} ${apply_arg}" | ||
| run "check/pylint-changed-files ${rev}" |
There was a problem hiding this comment.
With rev and apply_arg declared as arrays, we can pass arguments to the run function exactly (similar change needed on lines 105, 109 below):
| run "check/format-incremental ${rev} ${apply_arg}" | |
| run "check/pylint-changed-files ${rev}" | |
| run check/format-incremental "${rev[@]}" "${apply_arg[@]}" | |
| run check/pylint-changed-files "${rev[@]}" |
This is adapted from, but heavily modified from, Cirq's check/all.