Skip to content

Properly handle directories as arguments#713

Merged
nfischer merged 6 commits intomasterfrom
fix-cat-dir
May 9, 2017
Merged

Properly handle directories as arguments#713
nfischer merged 6 commits intomasterfrom
fix-cat-dir

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented May 1, 2017

We previously did not handle directories very well for many of our commands (would immediately error out). This fixes that.

  • cat(): did not handle directories as inputs, now it gracefully errors
  • head(): same thing. It also had a typo in a test.
  • sort(): same thing. It had the same typo.
  • tail(): same bug, and same typo.
  • uniq(): did not handle directories as inputs. Did not handle directories as outputs. Did not handle non-existent inputs (this slipped through because of a test typo). All are treated as errors.

Fixes #707

We also had a test which called sort() instead of uniq(), so we never
actually tested the missing-file case. This fixes that as well.

This also throws an error for using a directory as output.
@nfischer nfischer added the fix Bug/defect, or a fix for such a problem label May 1, 2017
@nfischer nfischer requested a review from freitagbr May 1, 2017 07:10
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #713 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #713      +/-   ##
==========================================
+ Coverage   94.66%   94.76%   +0.09%     
==========================================
  Files          33       33              
  Lines        1219     1241      +22     
==========================================
+ Hits         1154     1176      +22     
  Misses         65       65
Impacted Files Coverage Δ
src/tail.js 97.22% <100%> (+0.34%) ⬆️
src/uniq.js 100% <100%> (ø) ⬆️
src/head.js 94.73% <100%> (+0.39%) ⬆️
src/cat.js 100% <100%> (ø) ⬆️
src/sort.js 97.05% <100%> (+0.5%) ⬆️

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 bbe521b...505c503. Read the comment docs.

if (file !== '-') {
if (!fs.existsSync(file)) {
common.error('no such file or directory: ' + file, { continue: true });
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these return statements necessary? common.error throws an error, so the function should exit there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't throw an error with continue: true

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented May 6, 2017

@freitagbr PTAL

@freitagbr
Copy link
Copy Markdown
Contributor

LGTM

@nfischer nfischer merged commit a2e13b6 into master May 9, 2017
@freitagbr freitagbr deleted the fix-cat-dir branch May 10, 2017 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug/defect, or a fix for such a problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How am I suppose to handle errors with ShellJS?

3 participants