Skip to content

update error message #1992

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

Merged
merged 1 commit into from
Jan 26, 2019
Merged

update error message #1992

merged 1 commit into from
Jan 26, 2019

Conversation

lucask42
Copy link
Contributor

Also I removed a reference to system as a default alias - since as far as I know it is not a default

I ran into the same issue discussed in this closed issue: #1051

I wish I could contribute something more substantial to nvm. Thanks for maintaining this awesome project :)

Verified

This commit was signed with the committer’s verified signature.
ljharb Jordan Harband
@lucask42 lucask42 changed the title update error message and update error message Jan 24, 2019
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

nvm.sh Outdated
@@ -2322,7 +2322,7 @@ nvm() {
nvm_echo
nvm_echo 'Note: <version> refers to any version-like string nvm understands. This includes:'
nvm_echo ' - full or partial version numbers, starting with an optional "v" (0.10, v0.1.2, v1)'
nvm_echo " - default (built-in) aliases: ${NVM_NODE_PREFIX}, stable, unstable, ${NVM_IOJS_PREFIX}, system"
nvm_echo " - default (built-in) aliases: ${NVM_NODE_PREFIX}, stable, unstable, ${NVM_IOJS_PREFIX}"
Copy link
Member

Choose a reason for hiding this comment

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

"system" is indeed a built-in alias, that will only appear when you do have a system node installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha

nvm.sh Outdated
NVM_NODE_PREFIX="$(nvm_node_prefix)"
case "$1" in
"stable" | "unstable" | "${NVM_IOJS_PREFIX}" | "${NVM_NODE_PREFIX}")
nvm_err "${1-} is a default alias and cannot be deleted"
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 going to eventually change the terminology here from default to built-in, so let's use this wording:

Suggested change
nvm_err "${1-} is a default alias and cannot be deleted"
nvm_err "${1-} is a default (built-in) alias and cannot be deleted"

nvm.sh Outdated
@@ -3325,6 +3325,7 @@ nvm() {
;;
"unalias")
local NVM_ALIAS_DIR

Copy link
Collaborator

Choose a reason for hiding this comment

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

this line of change is not related

@lucask42
Copy link
Contributor Author

Thanks for the feedback!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great :-) can we add a test for this message?

@lucask42
Copy link
Contributor Author

I added a test - I'm not confident in my ability to write tests in Urchin so I peeked at the existing tests and used them as a guide.

What do you think?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Looks great!

@ljharb ljharb merged commit 02997b0 into nvm-sh:master Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants