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 includes the i flag #876
Conversation
includes the i flag to ignored upper/lower case differences
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 change looks good so far, but please also:
npm run gendocs
- Add a simple test in
test/grep.js
src/grep.js
Outdated
@@ -17,7 +18,8 @@ common.register('grep', _grep, { | |||
//@ Available options: | |||
//@ | |||
//@ + `-v`: Invert `regex_filter` (only print non-matching lines). | |||
//@ + `-l`: Print only filenames of matching files | |||
//@ + `-l`: Print only filenames of matching files. | |||
//@ + `-i`: Ignored upper/lower case differences. |
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.
nit: can you change this sentence to simply "Ignore case."?
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.
One comment in-line, otherwise LGTM
test/resources/case1
Outdated
@@ -0,0 +1 @@ | |||
Test3 |
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 OK, but could you move these files under test/resources/grep/case1*
? That should fix the other failures.
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.
OK, I moved it, and sorry for my thoughtless actions.
src/grep.js
Outdated
@@ -48,6 +50,9 @@ function _grep(options, regex, files) { | |||
} | |||
|
|||
var contents = file === '-' ? pipe : fs.readFileSync(file, 'utf8'); | |||
if (options.ignoreCase) { | |||
regex = new RegExp(regex, 'i'); |
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.
Sorry, one more fix: move the ignoreCase
logic out of the for-loop. The CI failure is because we're repeatedly re-assigning regex
, but we should only assign to it once.
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.
Oh, it's a low-level error, i fixed it, sorry.
Codecov Report
@@ Coverage Diff @@
## master #876 +/- ##
==========================================
+ Coverage 95.8% 95.81% +<.01%
==========================================
Files 34 34
Lines 1264 1266 +2
==========================================
+ Hits 1211 1213 +2
Misses 53 53
Continue to review full report at Codecov.
|
@ppsleep thanks for the great contribution! |
grep includes the i flag to ignored upper/lower case differences