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 check to nvm profile for nvm path #2467

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sladyn98
Copy link
Contributor

This PR aims to add a check for the profile before adding the path
Closes #2076

install.sh Outdated
Comment on lines 408 to 412
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
Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

@sladyn98
Copy link
Contributor Author

 ✓ nvm_check_global_modules
  ✓ nvm_detect_profile
  ✓ nvm_do_install
  ✓ nvm_install_dir
  ✓ nvm_install_with_aliased_dot
  ✓ nvm_install_with_node_version
  ✓ nvm_profile_is_bash_or_zsh
  ✓ nvm_reset
  ✓ nvm_source

Looking at the travis I see that the nvm_check_if_path_exists test does not run here

@ljharb
Copy link
Member

ljharb commented Apr 3, 2021

ah, you'll need to chmod a+x test/install_script/nvm_check_if_path_exists so that it can be executed :-)

@ljharb
Copy link
Member

ljharb commented Apr 15, 2021

I've rebased this, and marked it as a draft. Please mark it as ready for review when you think it's ready.

@ljharb ljharb marked this pull request as draft April 15, 2021 22:57
@sladyn98 sladyn98 closed this Jun 24, 2023
@ljharb ljharb reopened this Jun 25, 2023
@sladyn98
Copy link
Contributor Author

@ljharb Do you want me to complete this ?

@ljharb
Copy link
Member

ljharb commented Jun 28, 2023

@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.

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.

Option on install to not add path to .profile
2 participants