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.

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

Consolidate and avoid sed/cut calls, clean up pattern matches #718

Merged
merged 1 commit into from
Apr 7, 2015

Conversation

joliss
Copy link
Contributor

@joliss joliss commented Apr 6, 2015

Some patterns contained a no-op *; the * would match the empty string
because # or % replacement (unlike ## or %%) tries to find the shortest match.

(Follow up to #709.)

local NVM_NUM_DOTS
NVM_NUM_DOTS=$(echo "$VERSION" | command sed -e 's/[^\.]//g')
NVM_NUM_DOTS="${VERSION//[^.]/}"
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 afraid this form of substitution is not an option as it doesn't work in dash/sh - this will fail tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. I've reverted this one.

VERSION="${VERSION#v*}"
VERSION="${VERSION%\.}"
VERSION="${VERSION#v}"
VERSION="${VERSION%.}"
Copy link
Member

Choose a reason for hiding this comment

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

I've made a functionality change here so that "v" and "." report 0 in c34502e, and incorporated these changes there. Apologies for the merge conflict :-/ a fresh rebase should sort it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done; build is green now!

Some patterns contained a no-op `*`; the `*` would match the empty string
because # or % replacement (unlike ## or %%) tries to find the shortest match.
ljharb added a commit that referenced this pull request Apr 7, 2015
Consolidate and avoid sed/cut calls, clean up pattern matches
@ljharb ljharb merged commit 276d55c into nvm-sh:master Apr 7, 2015
@ljharb
Copy link
Member

ljharb commented Apr 7, 2015

Thanks!

@BenMorganIO
Copy link

Are there tests for this?

@ljharb
Copy link
Member

ljharb commented Apr 9, 2015

This commit hasn't been released yet, so your other issue wouldn't possibly be affected by it. But yes, there are tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants