-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
[New] Supercharge nvm debug
output
#1453
Conversation
Try to get shell version, OS and its version, curl/wget/git version.
70693bd
to
d113640
Compare
Output examples on different systems:
|
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.
Nice, I like it. Just one change.
nvm.sh
Outdated
@@ -2249,6 +2249,35 @@ nvm() { | |||
nvm_err "\$NPM_CONFIG_PREFIX: '$(nvm_sanitize_path "$NPM_CONFIG_PREFIX")'" | |||
nvm_err "\$NVM_NODEJS_ORG_MIRROR: '${NVM_NODEJS_ORG_MIRROR}'" | |||
nvm_err "\$NVM_IOJS_ORG_MIRROR: '${NVM_IOJS_ORG_MIRROR}'" | |||
nvm_err "shell version: '$(${SHELL} --version | command head -n 1)'" | |||
nvm_err "uname -a: '$(uname -a | awk '{$2=""; print}' | xargs)'" | |||
if [ "$(nvm_get_os)" = "darwin" ] && type sw_vers > /dev/null; then |
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.
can this use nvm_has
instead of type
?
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.
sure
2ec54e2
to
5f18eaa
Compare
Also added |
nvm.sh
Outdated
if nvm_has "curl"; then | ||
local CURL | ||
if type curl | command grep -q hashed; then | ||
CURL="$(type curl | command sed -E 's/\(|)//g' | command awk '{print $4}')" |
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.
it seems like this logic is repeated three times; can we consolidate it into a nvm_command_info
function? I'm thinking CURL="$(nvm_command_info curl)"
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.
done
cea67b7
to
97a3adf
Compare
Should be good now. |
[ "${WGET_COMMAND_INFO}" = "${WGET_EXPECTED_INFO}" ] || die "wget command info wrong(stage 2), expected: '${WGET_EXPECTED_INFO}', got '${WGET_COMMAND_INFO}'" | ||
|
||
# clean up this stage | ||
unalias wget |
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.
these two lines should probably just call cleanup
, no?
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.
Clean up will hide the output, I think we may not want to do this before the test finished.
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.
Hmm, good call. I'll update cleanup
itself to not hide the output.
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.
@ljharb I don't want the error message since if the alias is not there the cleanup
will give error message.
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'd prefer maximal output in tests; it helps when debugging a failure.
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 think we can treat the cleanup()
as the final process to do before exit but before that the cleanup process is a little bit different?
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.
Technically these three tests should each be in a separate file, each with their own cleanup.
[ "${WGET_COMMAND_INFO}" = "${WGET_EXPECTED_INFO}" ] || die "wget command info wrong(stage 1), expected: '${WGET_EXPECTED_INFO}', got '${WGET_COMMAND_INFO}'" | ||
|
||
# clean up this stage | ||
unset WGET_EXPECTED_INFO WGET_COMMAND_INFO |
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.
this line should just call cleanup
, no?
c7e6b22
to
7b253c8
Compare
Try to get shell version, OS and its version, curl/wget version.