Sheffield | 2026-MAR-SDC | Jak Rhodes-Smith | Sprint 3 | Implement shell tools#366
Sheffield | 2026-MAR-SDC | Jak Rhodes-Smith | Sprint 3 | Implement shell tools#366jakr-s wants to merge 11 commits into
Conversation
jakr-s
added
the
Needs Review
Poonam-raj
added
Review in progress
| const isBlank = line.trim() === ""; | ||
| const shouldNumber = options.n || (options.b && !isBlank); | ||
|
|
||
| console.log(shouldNumber ? `${lineNum} ${line}` : line); |
There was a problem hiding this comment.
To match the cat output accurately you would need to add the correct padding instead of just spaces. Here it's meant to be padding of 6 so the output is right-aligned.
| } finally { | ||
| await file.close(); |
There was a problem hiding this comment.
Nice use of finally to close the file after reading
|
|
||
| try { | ||
| let filePaths = program.args; | ||
| if (!filePaths || filePaths.length === 0) { |
There was a problem hiding this comment.
I would argue !filePaths is a bad path you weren't asked to account for
|
|
||
| const formatEntries = (files) => { | ||
| if (files.length === 0) return; | ||
| console.log(files.join(onePerLine ? "\n" : "\t")); |
There was a problem hiding this comment.
In the examples you've been asked to accommodate, there is always a newline option (-1) so the onePerLine check could be seen as unnecessary. (I'd argue you could've assumed onePerLine across this solution but I am treating this as a stretch behaviour to have it optional)
I agree the use of \t is an easier fix to calculating the column padding needed, but maybe less accurate to the real ls output columns.
|
|
||
| const filterHidden = (files) => files.filter((file) => !file.startsWith(".")); | ||
|
|
||
| const getVisibleEntries = (files) => |
There was a problem hiding this comment.
I personally dislike having a function like getVisibleEntries be dependent on a variable outside of this function. I'd prefer you to pass in includeHidden
It breaks the idea of "purity" (a function's output depends solely on arguments passed in or internal logic, no external dependencies - which includeHidden is a hidden dependency). It makes it hard to test (we need dependencies more loosely coupled), and harder to reuse
| let entries = [...result.files]; | ||
|
|
||
| for (const [dir, contents] of Object.entries(result.dirs)) { | ||
| const filtered = getVisibleEntries(contents); |
There was a problem hiding this comment.
(based on previous comment) we can see here how getVisibleEntries leaves a lot of questions - how is it determining what is visible or not? Is this a hard coded process, or is it a changeable output?
| formatEntries(result.files); | ||
|
|
||
| for (const [dir, contents] of Object.entries(result.dirs)) { | ||
| console.log("\n" + dir + ":"); |
There was a problem hiding this comment.
Small bug here - you're baking into your output a newline on the first line of prints - not an expected behaviour, the first line should be at the top with no newline
How can you separate out the concern so the newline gaps are between directory sections only and the top section doesn't have a newline before it?
2 participants
Learners, PR Template
Self checklist
Changelist
Complete all NodeJS tool implementations