feat(cat): number output lines (#750) #775
Conversation
src/cat.js
Outdated
| } | ||
|
|
||
| function number(n) { | ||
| return (' ' + n).slice(-6); |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
|
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.
I think lines.pop() is a bit clearer?
| t.is(result.toString(), 'test3'); | ||
| }); | ||
|
|
||
| test('empty', t => { |
There was a problem hiding this comment.
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.
Yes, it has work fine. I add these tests as part of the -n implementation.
Thanks for the feedback. :-)
Add
cat -nsupport to show number lines. Fix #750Additionally this add two tests: