-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: incorrect variable name display in must_get_env
error msg
#22615
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see any issue with that function as it is right now in develop
:
~/work/status-mobile develop 51s
> bash
[jakubgs@lilim:~/work/status-mobile]$ eval "$(sed -n '/function must_get_env/,+8p' scripts/build-android.sh)"
[jakubgs@lilim:~/work/status-mobile]$ must_get_env TEST
No required env variable: TEST
exit
jakubgs hey, thanks for checking! the problem is that in the error message you use If you run: unset TEST_VAR
must_get_env TEST_VAR the error shows
without the name. Changing it to just echo -e "${RED}No required env variable:${RST} ${BLD}$1${RST}" 1>&2 that way it shows:
|
@@ -3,16 +3,17 @@ | |||
# Needed to fail on must_get_env() | |||
set -e | |||
|
|||
GIT_ROOT=$(cd "${BASH_SOURCE%/*}" && git rev-parse --show-toplevel) | |||
GIT_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")" && git rev-parse --show-toplevel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that better?
is better in terms of clarity, correctness, and consistency, especially in more complex scripts or when the script could be sourced.
if [[ -n "${VAR_VALUE}" ]]; then | ||
echo "${VAR_VALUE}" | ||
return | ||
fi | ||
echo -e "${RED}No required env variable:${RST} ${BLD}${!VAR_VALUE}${RST}" 1>&2 | ||
echo -e "${RED}No required env variable:${RST} ${BLD}${VAR_NAME}${RST}" 1>&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !
trick works because VAR_VALUE
is not a variable but rather a reference to a var created via declare -n
:
-n make NAME a reference to the variable named by its value
Which means that VAR_VALUE
is a reference to $1
. We can test this:
> TEST=something
> VAR_NAME=TEST
> declare -n VAR_REF=$VAR_NAME
> echo $VAR_REF
something
> echo ${!VAR_REF}
TEST
Your version is clearer, but the result is the same. What I would like to understand is why for you it doesn't expand into the name of variable from the nameref
.
noticed a bug in the
must_get_env
function where the error message was showing the value reference (${!VAR_VALUE}
) instead of the actual variable name (${VAR_NAME}
).fixed it so that the error now correctly displays the name of the missing environment variable.