Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix install-release.sh by shellcheck #92

Closed
wants to merge 1 commit into from

Conversation

xiagw
Copy link
Contributor

@xiagw xiagw commented Sep 11, 2020

No description provided.

@@ -254,7 +254,7 @@ get_version() {
TMP_FILE="$(mktemp)"
install_software curl
# DO NOT QUOTE THESE `${PROXY}` VARIABLES!
if ! "curl" ${PROXY} -o "$TMP_FILE" 'https://api.github.com/repos/v2fly/v2ray-core/releases/latest'; then
if ! "curl" "${PROXY}" -o "$TMP_FILE" 'https://api.github.com/repos/v2fly/v2ray-core/releases/latest'; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO NOT QUOTE THESE ${PROXY} VARIABLES!

留意注释。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

看过这个变量赋值,
因为你用的赋值是没有空格的,(-xsocks5=xxxxx)
所以这里用quote 是没问题的,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是这样的,我过去套用 shellcheck 的 autofix 给这里加了双引号,造成实际传递 proxy 参数给脚本时,跑到这里会出错。

@@ -520,7 +516,7 @@ main() {
# Parameter information
[[ "$HELP" -eq '1' ]] && show_help
[[ "$CHECK" -eq '1' ]] && check_update
[[ "$REMOVE" -eq '1' ]] && remove_v2ray
[[ "$REMOVE" -eq '1' ]] && remove_v2ray "$REMOVE"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_v2ray 函数不需要任何参数。我不明白为什么你会在这里做这一更改。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是因为shellcheck会作为一个严重问题警告,
要么就去掉函数里的参数引用,
此处加上是为了忽略警告,(最小无害改动)

Copy link
Collaborator

@IceCodeNew IceCodeNew Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们写这个脚本不是为了通过 shellcheck 的警告,你不用把作为参考的建议当成必须解决的问题。
你提到说去掉函数里的参数引用,我现在在手机上不方便看,你把你说的 remove_v2ray 函数里有问题的地方引用出来给我看看呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

无他,shellcheck 给出的建议非常好而已,
给养成良好的习惯,就最大减少错误。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

在我看来这里的解决方案是从 remove_v2ray 函数里移除对 NAME 的赋值。
这个变量只在 install_file 函数里用到,而这个函数自己已经实现了对这个变量必要的赋值。

@IceCodeNew
Copy link
Collaborator

总的来说这个 PR 简化了很多逻辑,把原来用到其他系统命令的地方整合进 awk 的技巧我也很欣赏。
但实话说这个 PR 解决的都是 shellcheck 会给出的严重性较低的问题,这些问题在很多时候既不太可能因为跑进边边角角的例子里出错,也不构成维护上的困难。所以我并不急于合并这样的提交(因为现在确实没有很好的一个测试机制来确保这个 PR 不会造成问题)

从优化脚本的角度来说,我觉得有个 shellcheck 看不出来,但其实比较值得关注的问题是现在脚本对传入参数的实现。
目前的脚本对传入参数的处理太复杂了,其实一旦用到 shift 指令就能简单很多。现在这样的情况要新增一项命令行参数是很麻烦的。

如果你真的有心优化这个安装脚本,我会比较建议你从上面这个问题入手(我因为开学了所以最近没时间做这种体力活,工程有点繁大),
或者也可以帮忙解决一下 #89 问题。

Copy link
Collaborator

@IceCodeNew IceCodeNew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

见上面的讨论。

@xiagw
Copy link
Contributor Author

xiagw commented Sep 17, 2020

目前的脚本对传入参数的处理太复杂了,其实一旦用到 shift 指令就能简单很多。现在这样的情况要新增一项命令行参数是很麻烦的。

这个其实比较简单,一个while就可以搞定了。
再发一个 pr 给你吧。

@IceCodeNew
Copy link
Collaborator

这个 PR 里包含已经在 #108 里提交的修改。
且因为是往 master 分支的 PR,所以我决定关闭这个 PR。
请更新过本地的分支后,在最新的 develop 分支基础上重建这一 PR 的修改,然后重新 PR。

@IceCodeNew IceCodeNew closed this Sep 19, 2020
@xiagw xiagw deleted the patch-install-release-sh branch September 19, 2020 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants