Add support for nodenv #226

shaun-floss · 2019-11-15T04:58:08Z

This will add support for nodenv to display the current Node.js version in the prompt. I used the code from rbenv section as a reference to add this functionality.

I also updated the configuration setting theme_display_nvm to theme_display_nodejs. I left one reference to the old configuration setting so it will still work for anyone who is currently using it.

bobthecow · 2020-01-18T22:06:48Z

fish_prompt.fish

@@ -861,13 +861,31 @@ function __bobthefish_prompt_desk -S -d 'Display current desk environment'
 end

 function __bobthefish_prompt_nvm -S -d 'Display current node version through NVM'
-    [ "$theme_display_nvm" = 'yes' -a -n "$NVM_DIR" ]


I'm not sure about nodenv, but running nvm every time we render a prompt is slow. That's why it's opt-in rather than opt-out, and that's also why we check $NVM_DIR and bail early, rather than calling nvm current.

I'd love to support nodenv, but I really don't want to slow down the prompt for everyone all the time to do it :)

Thanks for responding, @bobthecow! 😃

running nvm every time we render a prompt is slow. That's why it's opt-in rather than opt-out, and that's also why we check $NVM_DIR and bail early, rather than calling nvm current.

That makes sense to me. Won't the new check/guard (right below this) ensure that we fail/bail early? 🤔

[ "$theme_display_nvm" = 'no' ] and return

Also, I can add the check for $NVM_DIR to the if type -fq nvm line if that works?

Let me know what you think! 😄

My point was that expensive checks should always be opt-in, not opt-out. And I'm not sure about nodenv, but nvm is definitely expensive, so it shouldn't be on by default.

Yes, the check for $NVM_DIR should be preserved. Maybe add it to the nvm branch, something like…

if type -fq nvm [ -n "$NVM_DIR" ] or return # ...

Yes, the check for $NVM_DIR should be preserved. Maybe add it to the nvm branch, something like…

if type -fq nvm [ -n "$NVM_DIR" ] or return # ...

I updated this to what you suggested. 😃

alessandro-fazzi

Other than my inline review, I'd like to say one thing about

this should be renamed to theme_display_node to be consistent with theme_display_ruby

IMO collapsing on theme_display_node will bring to - as it is brought by your PR - a single option enabling a single function full of branch conditions. Managing the node version could be done through nvm, nodeenv, asdf, you name it. Every version manager will have its own behavior, own plugins, etc.

Taking a trip on __bobthefish_prompt_rubies function: it relies on the theme_display_ruby option and it has 3 or 4 branch conditions inside. It happens just because you have to catch what version manager is the user on and implement specific logic.

While the function __bobthefish_prompt_nvm relies on theme_display_nvm option: if a user requires to enable nvm, I haven't to scrape his system in search for what version manager he's using; thus we can go straight to the only logic. This is easier to write, easier to read, easier to change.

Obviously you're proposing to follow an already used pattern; and yes, this is a design decision. So mine is just an opinion, but I'll obviously let the author drive the direction.

BTW, if you'll go for a rename, I spot that you shouldn't leave function __bobthefish_prompt_nvm untouched: if you're going to rename the option you should rename the function too. Shouldn't you?

alessandro-fazzi · 2020-02-19T08:25:07Z

fish_prompt.fish


-    set -l node_version (nvm current 2> /dev/null)
+    set -l node_version
+    if type -fq nvm


This could be a problem. Using my environment as an example:

 type -f nvm 373ms type: Could not find 'nvm'

but, since I've installed an OMF plugin...

 type nvm 185ms nvm is a function with definition # Defined in /Users/fuzzy/.local/share/omf/pkg/nvm/functions/nvm.fish @ line 1 function nvm --description 'Node version manager' if test -e $nvm_prefix/nvm.sh if not type -q fenv echo "You need to install foreign-env plugin" return 1 end fenv source $nvm_prefix/nvm.sh --no-use ';' nvm $argv else echo "You need to install nvm itself (see https://github.com/creationix/nvm#installation)" return 1 end end

As you can see I do have nvm but the binary executable is wrapped and masked by a function. This is how a lot of plugins work AFAIK.

My supposition is that type -f check could not be safe enough. Any thoughts?

Good point. In this case, type -q rather than type -fq seems appropriate.

Thanks for pointing this out! I updated it to type -q. 👍

bobthecow · 2020-05-02T15:47:31Z

fish_prompt.fish


-    set -l node_version (nvm current 2> /dev/null)
+    set -l node_version
+    if type -fq nvm


Good point. In this case, type -q rather than type -fq seems appropriate.

bobthecow · 2020-05-02T15:48:36Z

fish_prompt.fish

@@ -861,13 +861,31 @@ function __bobthefish_prompt_desk -S -d 'Display current desk environment'
 end

 function __bobthefish_prompt_nvm -S -d 'Display current node version through NVM'
-    [ "$theme_display_nvm" = 'yes' -a -n "$NVM_DIR" ]


My point was that expensive checks should always be opt-in, not opt-out. And I'm not sure about nodenv, but nvm is definitely expensive, so it shouldn't be on by default.

Yes, the check for $NVM_DIR should be preserved. Maybe add it to the nvm branch, something like…

if type -fq nvm [ -n "$NVM_DIR" ] or return # ...

This will make is so that the current Node.js version can be displayed using either nvm or nodenv. I also changed the configuration setting `theme_display_nvm` to `theme_display_nodejs`.

shaun-floss · 2020-06-07T23:49:31Z

Other than my inline review, I'd like to say one thing about

this should be renamed to theme_display_node to be consistent with theme_display_ruby

IMO collapsing on theme_display_node will bring to - as it is brought by your PR - a single option enabling a single function full of branch conditions. Managing the node version could be done through nvm, nodeenv, asdf, you name it. Every version manager will have its own behavior, own plugins, etc.

Taking a trip on __bobthefish_prompt_rubies function: it relies on the theme_display_ruby option and it has 3 or 4 branch conditions inside. It happens just because you have to catch what version manager is the user on and implement specific logic.

While the function __bobthefish_prompt_nvm relies on theme_display_nvm option: if a user requires to enable nvm, I haven't to scrape his system in search for what version manager he's using; thus we can go straight to the only logic. This is easier to write, easier to read, easier to change.

Obviously you're proposing to follow an already used pattern; and yes, this is a design decision. So mine is just an opinion, but I'll obviously let the author drive the direction.

BTW, if you'll go for a rename, I spot that you shouldn't leave function __bobthefish_prompt_nvm untouched: if you're going to rename the option you should rename the function too. Shouldn't you?

Thanks for reviewing and for the feedback! I ended up changing the configuration setting to theme_display_nodejs, I also update the documentation and related functions. Let me know what you think! 😃

shaun-floss · 2020-06-07T23:53:36Z

fish_prompt.fish

-function __bobthefish_prompt_nvm -S -d 'Display current node version through NVM'
-    [ "$theme_display_nvm" = 'yes' -a -n "$NVM_DIR" ]
+function __bobthefish_prompt_nodejs -S -d 'Display current Node.js version'
+    [ "$theme_display_nodejs" = 'yes' -o "$theme_display_nvm" = 'yes' ]
    or return


I left the existing configuration setting theme_display_nvm here for anyone who is currently using it. Let me know if this is okay, or if you want me to take another approach. 😃

bobthecow reviewed Jan 18, 2020

View reviewed changes

alessandro-fazzi reviewed Feb 19, 2020

View reviewed changes

bobthecow requested changes May 2, 2020

View reviewed changes

Add support for nodenv

02c7be2

This will make is so that the current Node.js version can be displayed using either nvm or nodenv. I also changed the configuration setting `theme_display_nvm` to `theme_display_nodejs`.

shaun-floss force-pushed the add-support-for-nodenv branch from 6f88208 to 02c7be2 Compare June 7, 2020 20:31

shaun-floss commented Jun 7, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nodenv #226

Add support for nodenv #226

shaun-floss commented Nov 15, 2019 •

edited

Loading

bobthecow Jan 18, 2020

shaun-floss Feb 16, 2020

bobthecow May 2, 2020

shaun-floss Jun 7, 2020

alessandro-fazzi left a comment

alessandro-fazzi Feb 19, 2020

bobthecow May 2, 2020

shaun-floss Jun 7, 2020

bobthecow May 2, 2020

bobthecow May 2, 2020

shaun-floss commented Jun 7, 2020

shaun-floss Jun 7, 2020

Add support for nodenv #226

Are you sure you want to change the base?

Add support for nodenv #226

Conversation

shaun-floss commented Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandro-fazzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaun-floss commented Jun 7, 2020

Choose a reason for hiding this comment

shaun-floss commented Nov 15, 2019 •

edited

Loading