-
-
Notifications
You must be signed in to change notification settings - Fork 113
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 NVM_LAZY_LOAD_EXTRA_COMMANDS option #67
Add NVM_LAZY_LOAD_EXTRA_COMMANDS option #67
Conversation
Closes #66 |
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.
Thanks for working on this!
Are you able to take another look at the test? It should probably be failing since the arrays aren't currently being merged properly.
|
||
# Set NVM_LAZY_LOAD to true | ||
export NVM_LAZY_LOAD=true | ||
export NVM_LAZY_LOAD_EXTRA_COMMANDS=('vim' 'ls') |
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.
vim
may not exist on all machines where we run tests and ls
could be called inside other functions inside zsh-nvm
.
For the test vectors can you choose commonly available yet not that widely used binaries, and things we are unlikely to ever call in zsh-nvm
.
whoami
and hostname
spring to mind.
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.
Good point, I'll use whoami
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 change ls
to hostname
too. Otherwise if we use ls
somewhere it could cause the test to start failing.
I've added the changes and amended the commit. |
@joerideg sorry, haven't had a chance to look at this, got some covid related craziness going on at my end. I haven't forgotten about it, I'll get round to it as soon as I'm able. |
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.
Apologies for the delay, I just got round to reviewing and testing this.
It works great, thanks!
If you could just make the minor tweaks I suggested this is good to be merged.
Use `hostname` instead of `ls` in test of NVM_LAZY_LOAD_EXTRA_COMMANDS option
@lukechilds I've updated the MR as requested. |
Perfect, thanks @joerideg! |
Here we go, I gave it a shot. It includes tests and documentation in README.md
Please review.