Fix issues reported by shellcheck #5910
Conversation
Exit when either of directory resolution or the cd command fail.
Use `-n` instead of `! -z`.
Convert implicitly-split strings to arrays.
Python 2 is not supported. Python 4 will not appear anytime soon.
Do not block `set -e` at variable assignment.
Convert rest to array as it hold command-line arguments.
| @@ -31,8 +31,9 @@ | |||
| ################################################################################ | |||
|
|
|||
| # Get the working directory to the repo root. | |||
| cd "$(dirname "${BASH_SOURCE[0]}")" | |||
There was a problem hiding this comment.
Why not cd "$(dirname "${BASH_SOURCE[0]}")" || exit?
My local shellcheck says ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
There was a problem hiding this comment.
That's what we are doing, i'm just curious about the creation of the thisdir variable.
There was a problem hiding this comment.
Why not cd "$(dirname "${BASH_SOURCE[0]}")"
Such cd would still pass for a failing $(...) command. If that command had no standard output,
the shell would expand it as cd "" and stay in the same working directory.
That could have unexpected effects further below.
Here is how to test -
$ cd /tmp
$ cd "$(false)" || echo "cd failed"There was a problem hiding this comment.
That's what we are doing, i'm just curious about the creation of the thisdir variable.
This allows us to fail when $(...) fails -
$ thisdir=$(false) || echo directory expression failed
| @@ -21,6 +21,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
When i run this file
(cirq_venv) vtomole@vtomole:~/Cirq$ shellcheck dev_tools/pypath
In dev_tools/pypath line 1:
# Copyright 2018 The Cirq Developers
^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang.There was a problem hiding this comment.
The dev_tools/pypath should work with both bash and zsh.
We can check it using
$ shellcheck --shell=bash dev_tools/pypath
Here is how I test all files, dev_tools/pypath is added separately, because file does not identify it as a shell script:
$ git ls-files | xargs file | grep Bourne-Again.shell.script | cut -d: -f1 |
xargs shellcheck --shell=bash dev_tools/pypathThere was a problem hiding this comment.
Can we add a shebang at the top so that it is identified as a shell script?
There was a problem hiding this comment.
Good point! Done in c44b6bf.
|
@vtomole - thank you for the review! PTAL again at your convenience. |
Help shellcheck to recognize bash syntax in dev_tools/pypath. Note the executable flag on `dev_tools/pypath` is deliberately off. The file is intended for sourcing from other shell sessions.
pavoljuhas
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
- exit on cd failure, SC2164
- needless double negative, SC2236
- fix useless use of cat, SC2002
- replace legacy backticks, SC2006
- clean unused variables, SC2034
- avoid globing/splitting on variable expansion, SC2086
- expand variables at trap execution, SC2064
- remove redundant loop over single value, SC2043
- export variable separately from value assignment, SC2155
- ${name} is not needed in arithmetic expressions, SC2004
- do not assign array to string, SC2124
- disable shellcheck for zsh block
- add shebang to `dev_tools/pypath` to identify it as a bash script
No change in code logic.
The updated code passes the following test
```
$ git ls-files | xargs file | grep -i shell.script | cut -d: -f1 |
xargs shellcheck
```
dev_tools/pypathto identify it as a bash scriptNo change in code logic.
The updated code passes the following test