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

[New] Print the version is going to be installed #1286

Merged
merged 1 commit into from
Nov 7, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Nov 6, 2016

For user experience enhancement/improvement

before:

$ nvm i 7
######################################################################## 100.0%
Computing checksum with sha256sum
Checksums matched!
Now using node v7.0.0 (npm v3.10.8)

$ nvm i 5
######################################################################## 100.0%
Computing checksum with sha256sum
Checksums matched!
Now using node v5.12.0 (npm v3.8.6)

after:

$ nvm i 7
Start to download and install node v7.0.0
######################################################################## 100.0%
Computing checksum with sha256sum
Checksums matched!
Now using node v7.0.0 (npm v3.10.8)

$ nvm i 5
Start to download and install node v5.12.0
######################################################################## 100.0%
Computing checksum with sha256sum
Checksums matched!
Now using node v5.12.0 (npm v3.8.6)

@PeterDaveHello
Copy link
Collaborator Author

Oops ... @ljharb do you have any suggestion about how to fix this?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

The failing tests check install output - you'll need to add your new nvm_echo line to the expected output.

@@ -1581,6 +1581,7 @@ nvm_install_binary() {
local TMPDIR
local VERSION_PATH

nvm_echo "Start to download and install ${FLAVOR} ${VERSION}"
Copy link
Member

Choose a reason for hiding this comment

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

The flavor is for internal use - otherwise this will be iojs iojs-v1.0.0 for example. To properly handle flavors, you'd need an if/else, with one message for node, and another for io.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I'll fix it, thanks.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb thanks for your review, may I have some hints about the output part? Thanks again.

@ljharb
Copy link
Member

ljharb commented Nov 6, 2016

@PeterDaveHello open the test files in question, look for the "EXPECTED_OUTPUT" variables, and add your new expected output

@PeterDaveHello
Copy link
Collaborator Author

@ljharb thank you so much! After tracing the code you mentions, it became much more clear, maybe we could also improve the expected and got error message to make it much more human readable in future 😄

@PeterDaveHello PeterDaveHello force-pushed the print-to-install-version branch 2 times, most recently from 6aeeced to 7a34767 Compare November 6, 2016 19:55
@PeterDaveHello
Copy link
Collaborator Author

Should be good now, waiting for the CI test!

@@ -1581,6 +1581,9 @@ nvm_install_binary() {
local TMPDIR
local VERSION_PATH

local NODE_OR_IOJS
[ "${FLAVOR}" = "node" ] && NODE_OR_IOJS="${FLAVOR}"
Copy link
Member

Choose a reason for hiding this comment

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

if set -e is set, won't this line error out?

I'd prefer a proper if here.

@@ -1581,6 +1581,9 @@ nvm_install_binary() {
local TMPDIR
local VERSION_PATH

local NODE_OR_IOJS
[ "${FLAVOR}" = "node" ] && NODE_OR_IOJS="${FLAVOR}"
nvm_echo "Start to download and install ${NODE_OR_IOJS} ${VERSION}"
Copy link
Member

Choose a reason for hiding this comment

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

to avoid errors at referencing unset variables, this needs to be ${NODE_OR_IOJS-} (note the dash)

Copy link
Member

Choose a reason for hiding this comment

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

Also, while we're making changes, let's have this copy be "Downloading and installing ${NODE_OR_IOJS-} ${VERSION}..." :-)

@PeterDaveHello
Copy link
Collaborator Author

@ljharb all done!

@ljharb ljharb added installing node Issues with installing node/io.js versions. feature requests I want a new feature in nvm! labels Nov 7, 2016
@PeterDaveHello PeterDaveHello force-pushed the print-to-install-version branch 2 times, most recently from a5df6de to c4648dd Compare November 7, 2016 06:15
@ljharb ljharb merged commit 9c92b5a into nvm-sh:master Nov 7, 2016
@PeterDaveHello PeterDaveHello deleted the print-to-install-version branch November 7, 2016 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants