Grep directory support - #1044
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #1044 +/- ##
==========================================
- Coverage 95.64% 95.61% -0.03%
==========================================
Files 35 35
Lines 1332 1347 +15
==========================================
+ Hits 1274 1288 +14
- Misses 58 59 +1
Continue to review full report at Codecov.
|
Sorry, something went wrong.
nfischer
left a comment
There was a problem hiding this comment.
This is cool! Thanks for writing this up. I left some initial comments. I haven't double-checked if the behavior is right though (I spot checked a couple spots, but I got different results than you in one of them), so you may want to look a bit closer there.
Either way, this seems like a good starting point for a valuable feature.
Sorry, something went wrong.
| test('works with array syntax', t => { | ||
| const result = shell.grep('-r', 'test', ['test/r*', 'package.json'], 'scripts'); | ||
| t.falsy(shell.error()); | ||
| t.is(result.split('\n').length, 72); |
There was a problem hiding this comment.
Is this the right value? I get a different answer in my shell:
$ grep -r 'test' test/r* package.json scripts | wc -l 69
Sorry, something went wrong.
| }); | ||
|
|
||
| test('works with array syntax', t => { | ||
| const result = shell.grep('-r', 'test', ['test/r*', 'package.json'], 'scripts'); |
There was a problem hiding this comment.
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?
Sorry, something went wrong.
| 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'); |
There was a problem hiding this comment.
I think we also need to check result.stdout. My grep will return results for the folders which actually do exist.
Sorry, something went wrong.
| t.is(result.split('\n').length, 29); | ||
| }); | ||
|
|
||
| test('multiple directories or files provided', t => { |
There was a problem hiding this comment.
We should probably split this into two separate test cases for clarity.
- We want a test case where multiple directories/files are provided, but they do not overlap. ex. "test/", "different-folder/file.txt"
- 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).
Sorry, something went wrong.
This Fixes #998