-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 check to nvm profile for nvm path #2467
base: master
Are you sure you want to change the base?
Conversation
install.sh
Outdated
if ${BASH_OR_ZSH} && ! command grep -qc '$NVM_DIR/.nvm' "$NVM_PROFILE"; then | ||
echo "=> Appending nvm path to $NVM_PROFILE" | ||
command printf "${SOURCE_STR}" >> "$NVM_PROFILE" | ||
else | ||
echo "=> nvm path already in ${NVM_PROFILE}" | ||
fi | ||
if ! command grep -qc '/nvm.sh' "$NVM_PROFILE"; 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 you elaborate a bit on this? it looks like this is basically just duplicating the block from lines 414-419.
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.
Oh yeah, I was thinking about the path checking logic so just thought that we would check if the NVM_DIR/.nvm
was in the profile for BASH
and ZSH
, we would avoid appending it else just append it. If i understood the issue correctly we would like to omit the addition of the path to the profile if it exists right ?
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.
Yes, but does line 414 not already do that?
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.
True how are we to then check the path ? I just thought that we would check it if its bash or zsh
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 guess I'm a bit confused. Why does it matter if it's bash or zsh? The original issue is implying that the current logic (which this PR does not change) is broken, and thus the "sourcing" lines are added more than once.
This implies that the existing check needs to be modified, rather than a new check added, for all shells.
Looking at the travis I see that the |
ah, you'll need to |
a609c96
to
c045303
Compare
I've rebased this, and marked it as a draft. Please mark it as ready for review when you think it's ready. |
@ljharb Do you want me to complete this ? |
@sladyn98 that'd be ideal :-) but if you're not interested then it's fine to leave it open so someone else can pick it up. |
c6cfc3a
to
c20db2a
Compare
This PR aims to add a check for the profile before adding the path
Closes #2076