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

Only filter the package npm, not any package name contains 'npm' #690

Merged
merged 2 commits into from
Mar 16, 2015

Conversation

hax
Copy link
Contributor

@hax hax commented Mar 11, 2015

No description provided.

@ljharb
Copy link
Member

ljharb commented Mar 11, 2015

@hax Thanks, great catch! Can you also modify the tests so they would have caught this?

@hax
Copy link
Contributor Author

hax commented Mar 12, 2015

It seems there are some other bugs because it report failure when I run tests on my laptop... I will try it on a clean vm later.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2015

Thanks - also feel free to push, and let travis-ci run them :-)

@@ -6,16 +6,17 @@ die () { echo "$@" ; exit 1; }

nvm use 0.10.28

EXPECTED_PACKAGES="autoprefixer bower david eslint grunt-cli grunth-cli http-server jshint marked node-gyp recursive-blame uglify-js yo"
EXPECTED_PACKAGES="autoprefixer bower david eslint grunt-cli grunth-cli http-server jshint marked node-gyp npmlist recursive-blame uglify-js yo"
Copy link
Member

Choose a reason for hiding this comment

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

Does npmlist require grunt-cli, which requires a new version of node? The tests run with 0.10 at the moment. Could we use another package that doesn't have those kind of requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npmlist use gulp not grunt.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then I'm not sure why the tests failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because I'm not familiar with shell programming...

@ljharb
Copy link
Member

ljharb commented Mar 14, 2015

Thanks for bearing with me! If the tests pass, please rebase this, freshly on top of master, down to no more than two commits (one for the test, one for the implementation) or one (for both), and I'll merge it. Your contribution is appreciated!

@ljharb
Copy link
Member

ljharb commented Mar 14, 2015

Almost there - rebasing it on top of the latest master should remove my two commits from your PR.

ljharb added a commit that referenced this pull request Mar 16, 2015
Only filter the package `npm`, not any package name that contains 'npm'
@ljharb ljharb merged commit 7750253 into nvm-sh:master Mar 16, 2015
@ljharb
Copy link
Member

ljharb commented Mar 16, 2015

@hax Thanks for the contribution!

@hax
Copy link
Contributor Author

hax commented Mar 16, 2015

There is still a problem -- should redo npm link for the linked packages, I will send a pr soon.

@hax hax deleted the patch-1 branch March 16, 2015 11:29
@ljharb
Copy link
Member

ljharb commented Mar 16, 2015

@hax that would definitely fix part of #341 :-)

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.

2 participants