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

[Fix] add missing quotes for $NODE_VERSION in nvm-exec #1321

Merged
merged 2 commits into from
Nov 25, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

No description provided.

@ljharb
Copy link
Member

ljharb commented Nov 24, 2016

Since NODE_VERSION has to be a proper semver single version number, is this something you've actually run into? Or is this just being careful?

If the latter, we might as well make one PR that quotes all mistakenly unquoted vars in the project.

@PeterDaveHello
Copy link
Collaborator Author

It's just being careful, and then we could try to let shellcheck involve in all the shellscripts.

@ljharb
Copy link
Member

ljharb commented Nov 24, 2016

If the goal here is to shellcheck nvm-exec, let's make the change in the same commit.

@PeterDaveHello
Copy link
Collaborator Author

Mean to let shellcheck involve in?

@PeterDaveHello
Copy link
Collaborator Author

I'm not sure if you'll like to disable SC1090 here?

In nvm-exec line 5:
\. "$DIR/nvm.sh" --no-use
^-- SC1090: Can't follow non-constant source. Use a directive to specify location.

@ljharb
Copy link
Member

ljharb commented Nov 24, 2016

yes, it should be disabled there just like it is in install.sh :-)

@PeterDaveHello
Copy link
Collaborator Author

Done, maybe you can just cancel the old builds :)

@PeterDaveHello
Copy link
Collaborator Author

Tests passed 👍

@ljharb ljharb merged commit 20ae7ee into nvm-sh:master Nov 25, 2016
@ljharb ljharb added the testing Stuff related to testing nvm itself. label Nov 25, 2016
@PeterDaveHello PeterDaveHello deleted the fix-nvm-exec branch November 25, 2016 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Stuff related to testing nvm itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants