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
feat(cat): number output lines (#750) #775
Conversation
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.
Thanks for the PR! Much appreciated.
src/cat.js
Outdated
} | ||
|
||
function number(n) { | ||
return (' ' + n).slice(-6); |
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.
Add a comment? I think this does the right thing (prepends 5 spaces for 1-digit numbers, 4 spaces for 2-digit, 3 spaces for 3-digit, etc.), but it's not super obvious. A one-line comment explaining this would be great.
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.
Also, can we rename this function? "number" is vague. Maybe we can rename this to "prependPaddedNumber" and change its functionality to accept both a number and line (and it will insert all padding).
src/cat.js
Outdated
} | ||
|
||
lines = lines.map(function (line, i) { | ||
return number(i + 1) + ' ' + line; |
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 GNU cat uses a tab, not a single space, between the number and line. It also makes for slightly easier parsing. I'm not sure if this is specified by POSIX, however.
test/cat.js
Outdated
t.is(result.toString(), ' 1 test3'); | ||
}); | ||
|
||
test('multiple numbers without EOF', 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.
Can we add a test to check a file with 10+ lines (just to check that padding works as expected)?
test/cat.js
Outdated
t.is(result.toString(), ' 1 test2\n 2 test1\n'); | ||
}); | ||
|
||
test('simple without EOF', 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.
Thanks for adding! Could you move this test above the numbers section? It would be nice to clump the -n
tests together.
src/cat.js
Outdated
var lines = cat.split('\n'); | ||
|
||
if (lines[lines.length - 1] === '') { | ||
addEOF = true; |
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.
Hmmm can we do something better here? It's kind of silly to strip the last array element simply to add it back at the end.
I'm not certain what the best approach is here.
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 can read by chunks from file stream. But even 'readline' package split
the buffer in lines.
https://github.com/nodejs/node/blob/master/lib/readline.js#L980
Codecov Report
@@ Coverage Diff @@
## master #775 +/- ##
=========================================
Coverage ? 95.39%
=========================================
Files ? 33
Lines ? 1237
Branches ? 0
=========================================
Hits ? 1180
Misses ? 57
Partials ? 0
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.
Couple more comments, otherwise LGTM. Thanks for the feature!
src/cat.js
Outdated
|
||
function addNumbers(cat) { | ||
var lines = cat.split('\n'); | ||
var lastLine = lines.splice(-1)[0]; |
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 lines.pop() is a bit clearer?
t.is(result.toString(), 'test3'); | ||
}); | ||
|
||
test('empty', 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.
Can you edit the PR description to mention that you're adding 2 additional tests for previous functionality? I assume this is code that worked before--please correct me if that's not so.
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.
Yes, it has work fine. I add these tests as part of the -n
implementation.
Thanks for the feedback. :-)
Add
cat -n
support to show number lines. Fix #750Additionally this add two tests: