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.

Sign up for GitHub

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 support for nodenv #226

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

Add support for nodenv #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ set -g theme_display_hg yes
set -g theme_display_virtualenv no
set -g theme_display_nix no
set -g theme_display_ruby no
set -g theme_display_nvm yes
set -g theme_display_nodejs yes
set -g theme_display_user ssh
set -g theme_display_hostname ssh
set -g theme_display_vi no
Expand Down Expand Up @@ -119,7 +119,7 @@ set -g theme_newline_prompt '$ '
**Prompt options**

- `theme_display_ruby`. Use `no` to completely hide all information about Ruby version. By default Ruby version displayed if there is the difference from default settings.
- `theme_display_nvm`. If set to `yes`, will display current NVM node version.
- `theme_display_nodejs`. If set to `yes`, will display current Node.js version.
- `theme_display_vagrant`. This feature is disabled by default, use `yes` to display Vagrant status in your prompt. Please note that only the VirtualBox and VMWare providers are supported.
- `theme_display_vi`. By default the vi mode indicator will be shown if vi or hybrid key bindings are enabled. Use `no` to hide the indicator, or `yes` to show the indicator.
- `theme_display_k8s_context`. This feature is disabled by default. Use `yes` to show the current kubernetes context (`> kubectl config current-context`).
Expand Down
35 changes: 28 additions & 7 deletions fish_prompt.fish
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
# set -g theme_display_hostname ssh
# set -g theme_display_sudo_user yes
# set -g theme_display_vi no
# set -g theme_display_nvm yes
# set -g theme_display_nodejs yes
# set -g theme_avoid_ambiguous_glyphs yes
# set -g theme_powerline_fonts no
# set -g theme_nerd_fonts yes
Expand Down Expand Up @@ -850,14 +850,35 @@ function __bobthefish_prompt_desk -S -d 'Display current desk environment'
set_color normal
end

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

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

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! 😄

Copy link
Member

Choose a reason for hiding this comment

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

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

  # ...

Copy link
Author

Choose a reason for hiding this comment

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

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

function __bobthefish_prompt_nodejs -S -d 'Display current Node.js version'
[ "$theme_display_nodejs" = 'yes' -o "$theme_display_nvm" = 'yes' ]
or return
Copy link
Author

Choose a reason for hiding this comment

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

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


set -l node_version (nvm current 2> /dev/null)
set -l node_version
if type -q nvm
[ -n "$NVM_DIR" ]
or return

[ -z $node_version -o "$node_version" = 'none' -o "$node_version" = 'system' ]
and return
set node_version (nvm current 2> /dev/null)

[ -z $node_version -o "$node_version" = 'none' -o "$node_version" = 'system' ]
and return
else if type -fq nodenv
set node_version (nodenv version-name)

# Don't show global node version...
set -q NODENV_ROOT
or set -l NODENV_ROOT $HOME/.nodenv

[ -e "$NODENV_ROOT/version" ]
and read -l global_node_version < "$NODENV_ROOT/version"

[ "$global_node_version" ]
or set -l global_node_version system

[ "$node_version" = "$global_node_version" ]
and return
end

__bobthefish_start_segment $color_nvm
echo -ns $node_glyph $node_version ' '
Expand Down Expand Up @@ -1076,7 +1097,7 @@ function fish_prompt -d 'bobthefish, a fish theme optimized for awesome'
__bobthefish_prompt_rubies
__bobthefish_prompt_virtualfish
__bobthefish_prompt_virtualgo
__bobthefish_prompt_nvm
__bobthefish_prompt_nodejs

set -l real_pwd (__bobthefish_pwd)

Expand Down