Address post-merge comments on PR #71 #74
Conversation
jo2y
commented
Apr 1, 2018
jo2y
commented
- Quote bash variable when possible.
- Fix an undefined $root_dir
- Remove a redundant rm and let the defined trap handler do it.
- echo -e is needed when trying to display escape codes.
- Remove incorrect {{foo}} double curly braces.
- Quote bash variable when possible.
- Fix an undefined $root_dir
- Remove a redundant rm and let the defined trap handler do it.
- echo -e is needed when trying to display escape codes.
- Remove incorrect {{foo}} double curly braces.
|
@rogerhub Take a look please. I resisted the urge to change all of the |
| tmp_dir=$(mktemp -d ${TMPDIR:-/tmp}/war.XXXXXXXX) | ||
| trap "{ cd ${root_path}; rm -rf ${tmp_dir}; }" EXIT | ||
| cd ${tmp_dir} | ||
| root_path="$(pwd)" |
There was a problem hiding this comment.
I don't think quotes are needed for the right-hand side of assignments. Also, this could be simplified to root_path=$PWD, right?
| cd ${tmp_dir} | ||
| root_path="$(pwd)" | ||
| tmp_dir=$(mktemp -d "${TMPDIR:-/tmp}/war.XXXXXXXX") | ||
| trap "{ cd "${root_path}"; rm -rf "${tmp_dir}"; }" EXIT |
There was a problem hiding this comment.
I think these double quotes aren't having their intended effect, since they're nested inside more double quotes. Ideally, this should look like:
cleanup() {
cd "${root_path}"
rm -rf "${tmp_dir}"
}
trap cleanup EXIT
That's a tad bit more complicated though, so I'll leave this up to your discretion.
| if [ -n "${1-}" ]; then | ||
| ${APP_ENGINE_ROOT}/bin/appcfg.sh -A "$1" update ${tmp_dir} | ||
| retCode=$? | ||
| "${APP_ENGINE_ROOT}/bin/appcfg.sh" -A "$1" update "${tmp_dir}" |
There was a problem hiding this comment.
Just a question: why is -A required in the python version but not here?
| trap - EXIT | ||
|
|
||
| exit $retCode | ||
| exit "$retCode" |
There was a problem hiding this comment.
optional: ret_code seems more consistent with other variables
| tmp_dir="$(mktemp -d "${TMPDIR:-/tmp}/war.XXXXXXXX")" | ||
| cp -R "$ROOT" "$tmp_dir" | ||
| trap "{ cd "$ROOT"; rm -rf "$tmp_dir"; }}" EXIT | ||
| rm -Rf "$tmp_dir/%{workspace_name}/external/com_google_appengine_py" |
There was a problem hiding this comment.
nit: lowercase -rf seems more consistent with other usages
| if [ -n "${{1-}}" ]; then | ||
| has_app_yaml=$(echo ${{@:2}} | grep -E "^([^ ]+ +)*app.yaml") | ||
| other_mod_cfgs=$(echo ${{@:2}} | xargs printf "%s\n" | grep -v "^app\.yaml$" | xargs echo) | ||
| ROOT="$PWD" |
There was a problem hiding this comment.
Maybe root_path to match the Java version?
| rm -Rf "$tmp_dir/%{workspace_name}/external/com_google_appengine_py" | ||
| if [ -n "${1-}" ]; then | ||
| has_app_yaml="$(echo "${@:2}" | grep -E "^([^ ]+ +)*app.yaml")" | ||
| other_mod_cfgs="$(echo "${@:2}" | xargs printf "%s\n" | grep -v "^app\.yaml$" | xargs echo)" |
There was a problem hiding this comment.
optional: Some of this code is brittle against paths with spaces in them, but users probably shouldn't be using those anyway. If you really care, there's probably a way to do all this in just bash. Maybe something like:
has_app_yaml=false
other_mod_cfgs=()
for p in "${@:2}"; do
if [[ "$p" == "app.yaml" ]]; then
has_app_yaml=true
else
modules+=("$p")
fi
doneAnd then you can use ${other_mod_cfgs[@]} to pass all of the other .yaml files to appcfg update below.
|
Hi, is anyone still looking at this? The python |
4 participants
