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

Grep directory support #1044

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -365,15 +365,15 @@ The main difference from `ls('-R', path)` is that the resulting file names
include the base directories (e.g., `lib/resources/file1` instead of just `file1`).


### grep([options,] regex_filter, file [, file ...])
### grep([options,] regex_filter, file_array)
### grep([options,] regex_filter, file [ ...file, ...directory])
### grep([options,] regex_filter, file_array,directory_array)

Available options:

+ `-v`: Invert `regex_filter` (only print non-matching lines).
+ `-l`: Print only filenames of matching files.
+ `-i`: Ignore case.

+ `-r`: recursive search
Examples:

```javascript
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
},
"dependencies": {
"execa": "^1.0.0",
"fs-readdir-recursive": "^1.1.0",
"glob": "^7.0.0",
"interpret": "^1.0.0",
"rechoir": "^0.6.2"
Expand Down
44 changes: 32 additions & 12 deletions src/grep.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
var common = require('./common');
var fs = require('fs');
var path = require('path');
var read = require('fs-readdir-recursive');


common.register('grep', _grep, {
globStart: 2, // don't glob-expand the regex
Expand All @@ -8,19 +11,20 @@ common.register('grep', _grep, {
'v': 'inverse',
'l': 'nameOnly',
'i': 'ignoreCase',
'r': 'recursive',
},
});

//@
//@ ### grep([options,] regex_filter, file [, file ...])
//@ ### grep([options,] regex_filter, file_array)
//@ ### grep([options,] regex_filter, file [ ...file, ...directory])
//@ ### grep([options,] regex_filter, file_array,directory_array)
//@
//@ Available options:
//@
//@ + `-v`: Invert `regex_filter` (only print non-matching lines).
//@ + `-l`: Print only filenames of matching files.
//@ + `-i`: Ignore case.
//@
//@ + `-r`: recursive search
//@ Examples:
//@
//@ ```javascript
Expand All @@ -31,29 +35,45 @@ common.register('grep', _grep, {
//@ Reads input string from given files and returns a
//@ [ShellString](#shellstringstr) containing all lines of the @ file that match
//@ the given `regex_filter`.
function _grep(options, regex, files) {
// Check if this is coming from a pipe
var pipe = common.readFromPipe();

if (!files && !pipe) common.error('no paths given', 2);
function getFilesInDir(directory) {
var files = read(directory);
for (var i = 0; i < files.length; i++) {
files[i] = path.join(directory, path.dirname(files[i]), path.basename(files[i]));
}
return files;
}

function _grep(options, regex, files) {
var pipe = common.readFromPipe();
if (!files && !pipe) common.error('No paths given', 2);
files = [].slice.call(arguments, 2);

if (pipe) {
files.unshift('-');
}

var grep = [];
var contents = '';
if (options.ignoreCase) {
regex = new RegExp(regex, 'i');
}
files.forEach(function (file) {

for (var i = 0; i < files.length; i++) {
var file = files[i];
if (!fs.existsSync(file) && file !== '-') {
common.error('no such file or directory: ' + file, 2, { continue: true });
common.error('no such file or directory: ' + file, 2, { silent: true });
} else if (!fs.existsSync(file) && file === '-') {
contents = pipe;
} else if (options.recursive && fs.statSync(file).isDirectory()) {
files = files.concat(getFilesInDir(file));
} else if (!options.recursive && fs.statSync(file).isDirectory()) {
common.error(file + ' Is a directory', 2, { silent: true });
return;
} else if (fs.statSync(file).isFile()) {
contents = fs.readFileSync(file, 'utf8');
}

var contents = file === '-' ? pipe : fs.readFileSync(file, 'utf8');
if (options.nameOnly) {
if (contents.match(regex)) {
grep.push(file);
Expand All @@ -67,12 +87,12 @@ function _grep(options, regex, files) {
}
});
}
});

}
if (grep.length === 0 && common.state.errorCode !== 2) {
// We didn't hit the error above, but pattern didn't match
common.error('', { silent: true });
}
return grep.join('\n') + '\n';
}

module.exports = _grep;
42 changes: 42 additions & 0 deletions test/grep.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,24 @@ test("multiple files, one doesn't exist, one doesn't match", t => {
t.is(result.code, 2);
});

test('-r option not provided, but directory is present', t => {
t.truthy(fs.existsSync('test/resources'));
t.truthy('/test/resources/a.txt');
const result = shell.grep(/oogabooga/, 'test/resources', 'test/resources/a.txt');
t.truthy(shell.error());
t.is(result.stderr, 'grep: test/resources Is a directory');
t.is(result.code, 2);
});

test('atleast one directory or file does not exist', t => {
t.truthy(fs.existsSync('test/resources'));
t.falsy(fs.existsSync('test/resources/random.txt'));
const result = shell.grep('-r', /oogabooga/, 'test/resources', 'test/resources/random.txt');
t.truthy(shell.error());
t.is(result.code, 2);
t.is(result.stderr, 'grep: no such file or directory: test/resources/random.txt');
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to check result.stdout. My grep will return results for the folders which actually do exist.

});

//
// Valids
//
Expand Down Expand Up @@ -162,6 +180,30 @@ test('-i option', t => {
t.is(result.split('\n').length - 1, 3);
});

test('one directory provided with -r option', t => {
const result = shell.grep('-r', 'test', 'test/resources');
t.falsy(shell.error());
t.is(result.split('\n').length, 29);
});

test('multiple directories or files provided', t => {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably split this into two separate test cases for clarity.

  1. We want a test case where multiple directories/files are provided, but they do not overlap. ex. "test/", "different-folder/file.txt"
  2. We want a test case (like this one) where there is overlap. We should assert that the overlapping files are double-counted (because that's what POSIX grep does). ex. "test/", "test/file.txt" (file.txt should be greped twice).

const result = shell.grep('-r', 'test', 'test/resources', 'test/resources/a.txt', 'test/resources/file2.txt', 'test/utils/');
t.falsy(shell.error());
t.is(result.split('\n').length, 39);
});

test('directory provided in glob form', t => {
const result = shell.grep('-r', 'test', 'test/r*');
t.falsy(shell.error());
t.is(result.split('\n').length, 66);
});

test('works with array syntax', t => {
const result = shell.grep('-r', 'test', ['test/r*', 'package.json'], 'scripts');
Copy link
Member

Choose a reason for hiding this comment

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

Please do not include anything outside of the resources/ folder in the test (package.json, scripts, etc.). It makes the test much more difficult to maintain if we do unrelated changes in package.json etc.

I think the same goes for test/r* because that matches test/rm.js. Instead of this, how about adding something like test/resources/grep2 and testing the glob test/resources/g* if you want to test globbing?

t.falsy(shell.error());
t.is(result.split('\n').length, 72);
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right value? I get a different answer in my shell:

$ grep -r 'test' test/r* package.json scripts | wc -l
69

});

test('the pattern looks like an option', t => {
const result = shell.grep('--', '-v', 'test/resources/grep/file2');
t.falsy(shell.error());
Expand Down