Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove uses of `expr` #461

Closed
wants to merge 4 commits into from

3 participants

@dolmen

4 commits that cleanup code by using more shell built-ins.

@ljharb ljharb commented on the diff
@@ -9,7 +9,6 @@ NVM_SCRIPT_SOURCE="$_"
nvm_has() {
type "$1" > /dev/null 2>&1
- return $?
@ljharb Collaborator
ljharb added a note

oops, duh, good call :-)

Oh, my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ljharb ljharb commented on the diff
@@ -429,14 +428,10 @@ nvm() {
nvm use $VERSION
if ! nvm_has "npm" ; then
echo "Installing npm..."
- if [ "`expr "$VERSION" : '\(^v0\.1\.\)'`" != '' ]; then
+ if [[ $VERSION == v0.1.* || $VERSION == v0.2.[0-2] ]]; then
@ljharb Collaborator
ljharb added a note

isn't "[[" a bashism? nvm.sh needs to work on all supported shells, including dash, ksh, zsh, etc.

@ljharb Collaborator
ljharb added a note

please note that this line is not covered by tests, so you'd need to test it manually

@dolmen
dolmen added a note

The second line of nvm.sh says:

# Implemented as a bash function
@ljharb Collaborator
ljharb added a note

Then that line is incorrect - I'll have to remove it.

@ljharb Collaborator
ljharb added a note

Comment fixed in 2ee4b6f

We probably should add some unit tests for this. What if you extract this to a function? Then adding a test is even easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ljharb ljharb referenced this pull request from a commit
@ljharb ljharb Removing unnecessary line, per #461 a7b6495
@ljharb
Collaborator

Thanks, I've manually included 2 out of your 4 commits (in the future, 3 separate PRs would be preferred) - please rebase on top of master, and make sure that nvm.sh works in all supported shells.

@ljharb ljharb changed the title from Code cleanup to Remove uses of `expr`
@ljharb
Collaborator

@dolmen do you have any interest in completing this PR? If not, I'd prefer to close this.

@ljharb
Collaborator

All of these uses of expr have been removed, in #526.

@ljharb ljharb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 9 deletions.
  1. +4 −9 nvm.sh
View
13 nvm.sh
@@ -9,7 +9,6 @@ NVM_SCRIPT_SOURCE="$_"
nvm_has() {
type "$1" > /dev/null 2>&1
- return $?
@ljharb Collaborator
ljharb added a note

oops, duh, good call :-)

Oh, my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
nvm_download() {
@@ -74,7 +73,7 @@ nvm_rc_version() {
local NVMRC_PATH
NVMRC_PATH="$(nvm_find_nvmrc)"
if [ -e "$NVMRC_PATH" ]; then
- NVM_RC_VERSION=`cat "$NVMRC_PATH" | head -n 1`
+ read NVM_RC_VERSION < "$NVMRC_PATH"
echo "Found '$NVMRC_PATH' with version <$NVM_RC_VERSION>"
fi
}
@@ -429,14 +428,10 @@ nvm() {
nvm use $VERSION
if ! nvm_has "npm" ; then
echo "Installing npm..."
- if [ "`expr "$VERSION" : '\(^v0\.1\.\)'`" != '' ]; then
+ if [[ $VERSION == v0.1.* || $VERSION == v0.2.[0-2] ]]; then
@ljharb Collaborator
ljharb added a note

isn't "[[" a bashism? nvm.sh needs to work on all supported shells, including dash, ksh, zsh, etc.

@ljharb Collaborator
ljharb added a note

please note that this line is not covered by tests, so you'd need to test it manually

@dolmen
dolmen added a note

The second line of nvm.sh says:

# Implemented as a bash function
@ljharb Collaborator
ljharb added a note

Then that line is incorrect - I'll have to remove it.

@ljharb Collaborator
ljharb added a note

Comment fixed in 2ee4b6f

We probably should add some unit tests for this. What if you extract this to a function? Then adding a test is even easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
echo "npm requires node v0.2.3 or higher" >&2
- elif [ "`expr "$VERSION" : '\(^v0\.2\.\)'`" != '' ]; then
- if [ "`expr "$VERSION" : '\(^v0\.2\.[0-2]$\)'`" != '' ]; then
- echo "npm requires node v0.2.3 or higher" >&2
- else
- nvm_download https://npmjs.org/install.sh -o - | clean=yes npm_install=0.2.19 sh
- fi
+ elif [[ $VERSION == v0.2.* ]]; then
+ nvm_download https://npmjs.org/install.sh -o - | clean=yes npm_install=0.2.19 sh
else
nvm_download https://npmjs.org/install.sh -o - | clean=yes sh
fi
Something went wrong with that request. Please try again.