NW | 2026-MAR-SDC | Ahmad Hmedan | Sprint 3 | Implement shell tool#452
NW | 2026-MAR-SDC | Ahmad Hmedan | Sprint 3 | Implement shell tool#452AhmadHmedann wants to merge 13 commits into
Conversation
AhmadHmedann
added
the
Needs Review
SlideGauge
added
Review in progress
SlideGauge
left a comment
SlideGauge
left a comment
There was a problem hiding this comment.
Good work, I have several notes from my side, implement them please.
| try { | ||
| const content = await fs.readFile(path, "utf-8"); | ||
| if(options.b) | ||
| { |
There was a problem hiding this comment.
what is javascript convention for braces placement?
do we place opening curly brace on the same line as the statement or not?
| const content = await fs.readFile(path, "utf-8"); | ||
| if(options.b) | ||
| { | ||
| const lines = content.split("\n"); |
There was a problem hiding this comment.
could you align the column on this line?
| let lineNumber = 1; | ||
|
|
||
| for (const path of paths) { | ||
| try { |
| for (const path of paths) { | ||
| try { | ||
| const content = await fs.readFile(path, "utf-8"); | ||
| if(options.b) |
There was a problem hiding this comment.
What if we decrease the complexity of the logic (amount of nested blocks) by extracting all the output logic into a separate function?
| .name("ls ") | ||
| .description("ls implementation") | ||
| .argument("[path]", "The path to process") | ||
| .option("-1, --one-per-line", "one file per line") |
There was a problem hiding this comment.
Do we have logic processing this parameter?
| }; | ||
|
|
||
|
|
||
| try { |
There was a problem hiding this comment.
the existing wc would continue execution if there was single error handling a single file, could we do the same here? (Just in case, cat.mjs does it)
| const content = await fs.readFile(path, "utf-8"); | ||
|
|
||
| const linesCounter = content.split("\n").length - 1; | ||
| const wordsCounter = content.trim().split(/\s+/).length; |
There was a problem hiding this comment.
what will happen with wordsCounter if the file content is empty?
| process.stdout.write(content); | ||
| } | ||
| } catch (error) { | ||
| console.error(error.message); |
There was a problem hiding this comment.
What will return real cat if at least one file had errors during processing?
| } | ||
|
|
||
| for (const line of lines) { | ||
| process.stdout.write(` ${lineNumber} ${line}\n`); |
There was a problem hiding this comment.
By default, cat -n right-justifies the line number in a 6-character-wide field, followed by a tab.
SlideGauge
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Implementation of cat, ls , and wc with support of require flag and multiple files.
Questions
Should I wrap each file operation in its own try–catch inside the loop, or use a single try–catch around the whole loop when processing multiple paths?