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

Auto install not exist version in zsh .nvmrc script, close #1272 #1306

Merged
merged 1 commit into from
Nov 24, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

No description provided.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb would you like to review this one? Thanks.

@@ -323,7 +323,9 @@ load-nvmrc() {
local nvmrc_node_version=$(nvm version "$(cat "${nvmrc_path}")")

if [ "$nvmrc_node_version" != "N/A" ] && [ "$nvmrc_node_version" != "$node_version" ]; then
nvm use
nvm use
Copy link
Member

Choose a reason for hiding this comment

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

this will run nvm install when [ "$nvmrc_node_version" = "$node_version" ], which isn't desired.

My comment meant that this should simply call nvm install, and not call nvm use at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just simply replace nvm use with nvm install here?

Copy link
Collaborator Author

@PeterDaveHello PeterDaveHello Nov 18, 2016

Choose a reason for hiding this comment

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

I thought directly use nvm install it may show something like v4.6.2 is already installed. if the version is already installed, but if you don't mind I'll be fine.

Copy link
Member

Choose a reason for hiding this comment

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

hm, yeah that's true. maybe it's best to run the appropriate command. this still needs to be tweaked tho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ljharb what about this?

      if nvm_is_version_installed "$nvmrc_node_version";                                                                                
        nvm use                                                                                                                         
      else                                                                                                                              
        nvm install                                                                                                                     
      fi

Copy link
Member

Choose a reason for hiding this comment

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

[ "$nvmrc_node_version" = "N/A" ] already tells you it's not installed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ohhhhhh ... I'm stupid lol ... hope this version is correct now:

    if [ "$nvmrc_node_version" = "N/A" ]; then                                                                                          
      nvm install                                                                                                                       
    elif [ "$nvmrc_node_version" != "$node_version" ]; then                                                                             
      nvm use                                                                                                                           
    fi

@PeterDaveHello PeterDaveHello force-pushed the zsh-script branch 2 times, most recently from 416b691 to 50fb098 Compare November 18, 2016 08:59
@PeterDaveHello
Copy link
Collaborator Author

@ljharb are we good to go here? Thanks.

@@ -323,7 +323,7 @@ load-nvmrc() {
local nvmrc_node_version=$(nvm version "$(cat "${nvmrc_path}")")

if [ "$nvmrc_node_version" != "N/A" ] && [ "$nvmrc_node_version" != "$node_version" ]; then
nvm use
nvm install
Copy link
Member

Choose a reason for hiding this comment

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

This will result in extra terminal output when it's already installed - although perhaps that's not a concern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's acceptable 👍

@ljharb ljharb merged commit e47b313 into nvm-sh:master Nov 24, 2016
@PeterDaveHello PeterDaveHello deleted the zsh-script branch November 24, 2016 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants