Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add nvm display to prompt. #23

Merged
merged 3 commits into from
Sep 7, 2020
Merged

Add nvm display to prompt. #23

merged 3 commits into from
Sep 7, 2020

Conversation

xolve
Copy link
Contributor

@xolve xolve commented Sep 6, 2020

Display node version when using nvm.

Motivation and Context

People keep multiple node versions using nvm. This helps in better visibility of shell environment.

Screenshots (if appropriate)

How Has This Been Tested

Tested with local nvm.

  • I have tested using MacOS.
  • I have tested using Linux.

Checklist

  • I have updated the documentation accordingly.
    Documentation has moved to wiki. Will update once PR is accepted.

- [ ] I have updated the tests accordingly.
Waiting for first review. Adding test cases not easy without running in a container.
Unnecessary for now

@IlanCosman
Copy link
Owner

It's great to see you again @xolve 😄 I felt badly about delaying your original PR for the major version bump. I'll review these changes and test them on MacOS. Good point about the documentation, I wish wikis were better integrated into their main repository.

@xolve
Copy link
Contributor Author

xolve commented Sep 6, 2020

😃 Had been busy with changes at work and had to take a break from OSS explorations. Well lets finish it this time.

@IlanCosman
Copy link
Owner

Thanks for the PR xolve. Your code was a really great starting point for the nvm item. Since I've rewritten most of the code, I'd like to discuss a few things about the original that you can improve on to create better fish scripts in the future 😄 In addition there were some style guide elements that were lacking.

General things:

  • There's no need to define top_level_dir inside _tide_node_version. Fish doesn't have private functions, so redefining top_level_dir every time is wasteful.
  • Helper functions like _tide_node_version and top_level_dir should generally come after the main function.
  • There isn't really a need for _tide_node_version to exist as a separate function. It is only run once and isn't long enough to be necessary to split off.

Details:

  • There's no need to use echo (node --version), that's the same as just using node --version.
  • No need for a ; after the else.
  • When you are printing tide_nvm_icon and node_version, what happens if the user erases the icon variable? You'll be left with a space hanging before node version, which will look silly. Use printf '%s' $tide_nvm_icon' ' $node_version. If tide_nvm_icon is not defined, that entire argument will be erased, fixing the problem of the extra space. We don't need a space after node_version because _tide_right_prompt will take care of that automatically.

Style guide things:

  • The indentation is a bit wonky, for example with the echos. You can use the builtin fish_indent command to fix that! There's also a great VSCode extension for fish that will do this automatically for you and provide syntax highlighting.
  • echo is considered to inferior to printf, which is why I discourage it in my project. 1 2
  • Local variable names should be in camelCase.

Non-code stuff:

  • I changed the default icon to the node logo: ⬢.
  • The config files are organized alphabetically, so I moved the nvm variables to the correct place. I use the sort lines VSCode extension for this.

Other changes:

  • I added a new variable tide_nvm_default_node. This is useful for those who use Jorgebucaran's nvm.fish. nvm.fish doesn't have a conception of the "system" version, so tide_nvm_default_node can be used to prevent the nvm item from displaying when using a certain node version, set by the user.

I still need to test this item on MacOS.

As for automated tests, I'm planning to introduce command mocking soon that will make it much easier to write test for items like this, so we can ignore them for now.

Can you review and briefly tests my changes and see if they work for you?

I really appreciate your work, sorry for the wall of text 😂

@IlanCosman
Copy link
Owner

I've tested the item on MacOS and made a few changes. I can handle the documentation since I added a new variable and there are a few oddities to discuss. Thanks @xolve 😄

@IlanCosman IlanCosman merged commit e9baf3a into IlanCosman:main Sep 7, 2020
@xolve
Copy link
Contributor Author

xolve commented Sep 14, 2020

Thanks for massive code clean up 😄 I do get the part where I missed to give option to set NVM_HOME.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants