-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
nvm.fish: add page #7512
nvm.fish: add page #7512
Conversation
Hello! I've noticed something unusual when checking this PR:
Is this intended? If so, just ignore this comment. Otherwise, please double-check the commits. |
pages/common/nvm.fish.md
Outdated
@@ -0,0 +1,25 @@ | |||
# nvm.fish |
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.
If I understand correctly, the binary is nvm
so I think there is a mismatch here.
Moreover, the CLI interface seems similar to https://github.com/nvm-sh/nvm so I am not sure this whole page is needed (it seems to overlap with #7511). 💭
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.
There are several differences between the original nvm
with this version. In short, currently the commands between both versions aren't compatible with each other:
- No
nvm run
ornvm exec
(though I can just create a PR for that 😀) - The default Node.js version is set through
set
instead of a subcommand (again, even though I can just PR about it 😅)
Maybe I should close this PR by sending a couple to https://github.com/jorgebucaran/nvm.fish...
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 the detailed information. 👍
I understand that commands are different. 👌
But I don't understand what to do with the mismatch between the binary name (nvm
) and the page name (we would need to run tldr nvm.fish
to find it, if I understand correctly). 🤔
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.
There's a bunch of related discussions related to this:
- Help documenting other variants of NVM (Node Version Manager) #7480 - The one I've referenced here
- Group commands by shells #7503 - Plans to separate commands by shells, if so I can move this into something like
common/fish/nvm
- Let's document: PowerShell #6621 - PowerShell, related to Group commands by shells #7503
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.
The title of a page needs to exactly match the command name being executed. This is not the case for the filename.
- Filename:
nvm.fish.md
- Page title:
nvm
- Command being executed:
nvm
Ideally the page title and the filename will match, but this is unfortunately not always possible.
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.
For completeness, the command in question where it's also not possible is rename
.
Co-authored-by: CleanMachine1 <78213164+CleanMachine1@users.noreply.github.com>
|
||
- Set the default Node.js version: | ||
|
||
`set nvm_default_version {{node_version}}` |
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.
Is this a typo? Should this be prefixed by nvm
at all, or is this essentially updating an environment variable that nvm
(fish) reads?
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.
Yes it uses the Fish' builtin set
command to set the default version.
Hi all! This thread has not had any recent activity. |
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, @reinhart1010!
common/
,linux/
, etc.)Version of the command being documented (if known): 2.2.6
This is related to our discussion on #7480. However, this might break some tldr clients as this is the first known file to have more than one dot characters (
.
).