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

Add info to restart bash if verification not working in verification section #1749

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

asab-se
Copy link
Contributor

@asab-se asab-se commented Feb 25, 2018

Just added the info to restart the bash terminal if command -v nvm does not ouput anything or gives an error after installation. It was mentioned somewhere earlier but when I read it, I just took note of the verification section, not the part before where the info was mentioned.

@PeterDaveHello
Copy link
Collaborator

I think all the installation methods already mention this:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

Right?

@asab-se
Copy link
Contributor Author

asab-se commented Feb 26, 2018

Just wanted to give a reason why to restart the bash for non-experienced users instead of just saying to restart the bash for no reason. Or what do you mean?

@PeterDaveHello
Copy link
Collaborator

Actually the comment after the 2nd and 3rd line explains it?

@asab-se
Copy link
Contributor Author

asab-se commented Feb 28, 2018

Do you mean the whole addition? "Note: Please note that if nvm is not outputted or with the error nvm: command not found, you might want to restart the bash and try it in a fresh bash terminal again, since there are a few variables exported which need to be initialized in a fresh bash."

Or just the part , since there are a few variables exported which need to be initialized in a fresh bash. ?

When I installed nvm the verification command -v nvm did not show nvm as output because I had to restart my bash.

@PeterDaveHello
Copy link
Collaborator

For example, here is the installation message from the scripting method:

=> Close and reopen your terminal to start using nvm or run the following to use it now:

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

It clearly mentions to restart the terminal or run those 3 commands, right?

@asab-se
Copy link
Contributor Author

asab-se commented Mar 1, 2018

Okay, I see. But the problem I see is that you provide a readme with an installation guide, so a user (I am one of them) will probably just use the instructions from the readme and will not take much note about the installation echos (I am just watching for errors and warnings in this case). In the readme within the installation section there is a header called "Verify installation" so the user will type in the command to verify if everything worked properly and, of course, nvm won't show up.
I ran into the same issue and just restarted the bash due to the experience I have that this might be the reason, not because I read the output of the installation script. Others might not and then will be confused because there is nothing more mentioned in the Verify installation section even though it is the expected behaviour that nvm won't show up without restarting the bash or running the commands you mentioned. I would not mix the lack of information in the readme with the infos provided during the installation process itself.

Therefore I would either move the note from few lines above the "Verifiy installation" section to this section or add the stuff I added to it. The note I mean is:


"Note: On Linux, after running the install script, if you get nvm: command not found or see no feedback from your terminal after you type:

command -v nvm

simply close your current terminal, open a new terminal, and try verifying again."


@PeterDaveHello
Copy link
Collaborator

PeterDaveHello commented Mar 1, 2018

I'm not sure if we should maintain the prompt at multi places, most users of nvm should be developers IMO, should better to take a look at the output, not just copy and paste, let's see what does @ljharb say.

@ljharb
Copy link
Member

ljharb commented Apr 10, 2018

If adding a line in the readme will help, then it might be worth it. However, I think it's quite reasonable to assume you're reading every line in the console, since only developers would be installing node.

@asab-se can you update this PR to use your amended note?

@asab-se asab-se closed this Jul 23, 2019
@asab-se asab-se deleted the add-info-to-readme branch July 23, 2019 18:39
@ljharb
Copy link
Member

ljharb commented Jul 23, 2019

@asab-se are you not interested in updating this?

@asab-se
Copy link
Contributor Author

asab-se commented Jul 23, 2019

Oh sorry, had to clean up a bit after having had a complete mess by having different accounts. I can update this ofc if interested. Smh thought I did this a while ago actually 🙈

@ljharb ljharb merged commit 47f0b32 into nvm-sh:master Sep 29, 2021
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.

3 participants