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

Add sudo support for npm installer. #216

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

weakish
Copy link
Contributor

@weakish weakish commented May 3, 2016

No description provided.

@joshgoebel
Copy link
Contributor

What is fgrep? This has to work on a ton of diff platforms. OS X, other BSDs, Linux and its proliferation of distros. Unless there is a big reason to change grep is probably safer, no?

@weakish
Copy link
Contributor Author

weakish commented May 6, 2016

What is fgrep?

Same as grep -F.
It matches fixed string instead of regex.
That is why it is much faster.

This has to work on a ton of diff platforms. OS X, other BSDs, Linux and its proliferation of distros.

fgrep is available on Linux, OS X, BSDs (even on busybox).

grep is probably safer

Since fgrep is just a shortcut to grep -F, they are equally safe.

Anywhere, fgrep is deprecated.
I will change it to grep -F.

`fgrep` is deprecated according to `man grep`.
end
end

verify_api do
def has_npm(package)
@commands << "npm --global list | grep \"#{package}@\""
@commands << "npm --global list | grep -F \"#{package}@\""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think to be correct her we need to match " [package]" (space in front), based on the output - to avoid false matches, yes? I'd say fix this and we can merge.

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.

2 participants