Glasgow | 25-SDC-July | Sheetal Kharab | Sprint 3 | Implement shell tools#131
Glasgow | 25-SDC-July | Sheetal Kharab | Sprint 3 | Implement shell tools#131sheetalkharab wants to merge 18 commits intoCodeYourFuture:mainfrom
Conversation
sheetalkharab
added
the
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great start with these tasks. I've left some comments on your files for where you could improve them more.
implement-shell-tools/cat/cat.js
Outdated
|
|
||
| for (const path of argv) { | ||
| try { | ||
| const content = await fs.readFile(path, "utf-8"); |
There was a problem hiding this comment.
Be careful of the indentation here - try to make it consistent across a file.
There was a problem hiding this comment.
@LonMcGregor thank you for review and added the extension for indentation checking .
implement-shell-tools/cat/cat.js
Outdated
| if (line.trim() === "") { | ||
| console.log(""); // Blank line, no number | ||
| } else { | ||
| const lineNumber = String(lineCounter++).padStart(6, " "); |
There was a problem hiding this comment.
You duplicating your code a bit when printing out with line numbers, can you think of a way to reduce this duplication?
There was a problem hiding this comment.
I tried to use a function to remove duplication of code.
implement-shell-tools/wc/wc.js
Outdated
|
|
||
| if (files.length > 1) { | ||
| if (options.lines) { | ||
| console.log(`${totalLines} total`)//if more then 1 file is |
There was a problem hiding this comment.
There is some duplication when you are printing out your total and counts. Can you think how to reduce that?
There was a problem hiding this comment.
thank you . try to make change as you suggested.
LonMcGregor
added
Reviewed
sheetalkharab
added
the
Needs Review
|
Great work on reducing duplication - your wc implementation in particular is much easier to read now. Further testing with your cat implementation reveals a problem with how it is handling some of the program options. Have you fully tested these / compared them to the original cat program? |
LonMcGregor
removed
the
Needs Review
sheetalkharab
added
the
Complete
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
2 participants
Learners, PR Template
Self checklist
Changelist
add code for cat wc and ls
Questions
Ask any questions you have for your reviewer.