Update build-zip.sh: Support for the QUALIFICATION of nativecode #10
Conversation
zaoqi
commented
Jul 5, 2019
•
zaoqi
commented
- Put quotes around all variables
- Use curl instead of wget
- The configuration language becomes a bash DSL that supports more complex configurations.
- Support:
- ...
There was a problem hiding this comment.
The feature you want to implement is interesting. Thanks for submiting it. But I think the implementation could be improved.
I can understand why you did some changes like querying package's version name instead of the version code, but I cannot understand why you replaced wget for cURL and rewrote the Main section. Contributing to a codebase shouldn't be rewriting things for no apparent reason, but to discuss changes. We are open to that.
Also, for that same reason, please use commits appropriately: each change should be a standalone commit. That way we could discuss them separately. It would help to explain why you do some changes in the PR message, too, like the cURL change. Sorry, but I won't review further PR with a single commit for multiple changes, it's time-consuming.
Finally, thanks for pointing at the unquoted variables issue and fixing it!
build-zip.sh
Outdated
| --output-document=$FILENAME \ | ||
| $URL | ||
| } && export -f fetch | ||
| curl -o "$FILENAME" "$URL" |
There was a problem hiding this comment.
Why using cURL? I'm not against, but downloading a file is a simple usecase that wget supports just fine.
Also, observe how we used command flags/options. Longer names are preferred for readability.
There was a problem hiding this comment.
Because busybox's wget does not support https
There was a problem hiding this comment.
Use Termux's wget, as stated in termux/termux-packages#2423
| @@ -38,62 +31,65 @@ function get_repo_base_url() { | |||
| esac | |||
|
|
|||
| echo $BASE_URL | |||
| } && export -f get_repo_base_url | |||
| } | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
to support complex configurations and remove GNU awk, xargs requirements
build-zip.sh
Outdated
|
|
||
| local REPO_URL=$(get_repo_base_url $REPO_NAME) | ||
| local REPO_URL="$(get_repo_base_url "$REPO_NAME")" |
There was a problem hiding this comment.
Are outer quotes really necessary? (honestly, I don't recall it myself)
