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
base: master
Are you sure you want to change the base?
Grep directory support #1044
Conversation
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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('works with array syntax', t => { | ||
const result = shell.grep('-r', 'test', ['test/r*', 'package.json'], 'scripts'); |
There was a problem hiding this comment.
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?
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.
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.
t.is(result.split('\n').length, 29); | ||
}); | ||
|
||
test('multiple directories or files provided', t => { |
There was a problem hiding this comment.
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.
- 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 begrep
ed twice).
This Fixes #998