fix: Exit 1 with empty string if no match #901

wyardley · 2018-11-08T07:35:11Z

Resolve an issue where grep returned 0 (with a \n as the output), even when the regex doesn't match.

Fixes #900

wyardley · 2018-11-08T07:38:20Z

@nfischer
The failure is on those other two test cases on line 39 and 48; seems to be returning both the original error, and the new one, even with that explicit return.

   38:   t.truthy(shell.error());                                          
   39:   t.is(result.stderr, 'grep: no such file or directory: /asdfasdf');
   40:   t.is(result.code, 2);                                             

  Difference:

  - `grep: no such file or directory: /asdfasdf␊
  - grep: `
  + `grep: no such file or directory: /asdfasdf

nfischer · 2018-11-08T07:42:32Z

@wyardley can you add "Fixes #900" to the PR description?

nfischer

Looks pretty close so far. Let me know if explanations make sense, since we don't have much documentation on these common APIs.

src/grep.js

wyardley · 2018-11-08T15:09:41Z

test/grep.js

@@ -127,6 +134,15 @@ test('one file, * in string-regex, make sure * is not globbed', t => {
  t.is(result.toString(), 'this line ends in.js\nlllllllllllllllll.js\n');
 });

+test("one file, pattern doesn't match", t => {


Do you want this under 'valids' or 'invalids?
Even though it's a non-0 exit code, I would personally consider testing for the absence of something to be a "valid" use case, but I'm happy to move it.

This spot LGTM

wyardley · 2018-11-08T15:12:33Z

@nfischer Ok, how's that look. I added another test case for the specific case where there are two files, one which dne and the other of which doesn't match. I can move this to the "Invalids" section if you think that's better (see above)...

codecov-io · 2018-11-08T16:20:48Z

Codecov Report

Merging #901 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
+ Coverage   97.32%   97.32%   +<.01%     
==========================================
  Files          34       34              
  Lines        1269     1271       +2     
==========================================
+ Hits         1235     1237       +2     
  Misses         34       34

Impacted Files	Coverage Δ
src/grep.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b3c7b1...163e049. Read the comment docs.

nfischer

LGTM % suggestions

test/grep.js

nfischer · 2018-11-09T21:50:51Z

test/grep.js

@@ -127,6 +134,15 @@ test('one file, * in string-regex, make sure * is not globbed', t => {
  t.is(result.toString(), 'this line ends in.js\nlllllllllllllllll.js\n');
 });

+test("one file, pattern doesn't match", t => {


This spot LGTM

test/grep.js

When we can read all files, but none match, exit 1 and return no output. This matches the behavior of grep more closely. Fixes #900

wyardley · 2018-11-09T22:45:34Z

@nfischer applied your suggestions and squashed

nfischer · 2018-11-13T04:26:06Z

Thanks. I'll merge this after I push a release (this probably constitutes a breaking change).

Nel75 · 2021-04-06T19:51:29Z

Hi. This fix is still not available on the latest release v0.8.4, despite being on the master branch (https://github.com/shelljs/shelljs/blob/master/src/grep.js#L72-L75). What is its status? Was there an issue with its packaging? This fix is really needed, as the current grep implementation is broken - I would argue it's a fixing change rather than a breaking one, as anyone relying on exit code 1 to detect the absence of matches is currently broken. Thanks.

nfischer · 2021-04-06T20:38:41Z

I think this landed after v0.8.3 went out. v0.8.4 was a small patch release for a specific bug, which is why this didn't make it in that release either.

I've updated the milestone field to clarify this is expected to be part of v0.9.0. I haven't had time to figure out what needs to make it in that release and what should be punted to the next release.

Nel75 · 2021-04-09T14:50:07Z

@nfischer Thanks for the quick reply. Any idea when v0.9.0 might be released? It's been over 2 years now since this fix, so I'll have to use another script in the mean time.

nfischer requested changes Nov 8, 2018

View reviewed changes

src/grep.js Outdated Show resolved Hide resolved

src/grep.js Outdated Show resolved Hide resolved

src/grep.js Outdated Show resolved Hide resolved

wyardley changed the title ~~fix: Exit 1 with empty string if no match (#900)~~ fix: Exit 1 with empty string if no match Nov 8, 2018

wyardley commented Nov 8, 2018

View reviewed changes

nfischer approved these changes Nov 9, 2018

View reviewed changes

fix: Exit 1 with no output if no match (#900)

163e049

When we can read all files, but none match, exit 1 and return no output. This matches the behavior of grep more closely. Fixes #900

nfischer added the fix Bug/defect, or a fix for such a problem label Nov 9, 2018

nfischer merged commit 5da1dda into shelljs:master Nov 13, 2018

wyardley deleted the wyardley-issue_900_grep branch November 13, 2018 05:37

nfischer mentioned this pull request Dec 2, 2018

chore(node): drop node v4 and v5 (#917) shelljs/shx#157

Merged

nfischer added this to the v0.9.0 milestone Apr 6, 2021

nfischer added the breaking Breaking change label Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Exit 1 with empty string if no match #901

fix: Exit 1 with empty string if no match #901

wyardley commented Nov 8, 2018 •

edited

wyardley commented Nov 8, 2018 •

edited

nfischer commented Nov 8, 2018

nfischer left a comment

wyardley Nov 8, 2018

nfischer Nov 9, 2018

wyardley commented Nov 8, 2018 •

edited

codecov-io commented Nov 8, 2018 •

edited

nfischer left a comment

nfischer Nov 9, 2018

wyardley commented Nov 9, 2018

nfischer commented Nov 13, 2018

Nel75 commented Apr 6, 2021 •

edited

nfischer commented Apr 6, 2021

Nel75 commented Apr 9, 2021

fix: Exit 1 with empty string if no match #901

fix: Exit 1 with empty string if no match #901

Conversation

wyardley commented Nov 8, 2018 • edited

wyardley commented Nov 8, 2018 • edited

nfischer commented Nov 8, 2018

nfischer left a comment

Choose a reason for hiding this comment

wyardley Nov 8, 2018

Choose a reason for hiding this comment

nfischer Nov 9, 2018

Choose a reason for hiding this comment

wyardley commented Nov 8, 2018 • edited

codecov-io commented Nov 8, 2018 • edited

Codecov Report

nfischer left a comment

Choose a reason for hiding this comment

nfischer Nov 9, 2018

Choose a reason for hiding this comment

wyardley commented Nov 9, 2018

nfischer commented Nov 13, 2018

Nel75 commented Apr 6, 2021 • edited

nfischer commented Apr 6, 2021

Nel75 commented Apr 9, 2021

wyardley commented Nov 8, 2018 •

edited

wyardley commented Nov 8, 2018 •

edited

wyardley commented Nov 8, 2018 •

edited

codecov-io commented Nov 8, 2018 •

edited

Nel75 commented Apr 6, 2021 •

edited