WestMidlands | SDC-NOV-2025 | SARA TAHIR | Sprint 3 | Implement Shell Tools #243
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-3) doesn't match expected format (example: 'Sprint 2', without quotes) 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
SaraTahir28
changed the title
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-3) doesn't match expected format (example: 'Sprint 2', without quotes) 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
SaraTahir28
changed the title
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
SaraTahir28
changed the title
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
SaraTahir28
added
the
Needs Review
…ypes in terminal.
…with node to mimic ls
SaraTahir28
force-pushed
the
Feature/ImplementShell-Nodejs
branch
from
9983327 to
6152444
Compare
SaraTahir28
changed the title
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work on these tasks, sorry for the late review
Good use of comments to explain your code
I've left comments in some areas where you could improve your code
| const paddingWidth = 6; // width for line number padding | ||
|
|
||
| // Helper function to format a line with numbering | ||
| function formatLine(line, lineNumber, numberAll, numberNonBlank) { |
There was a problem hiding this comment.
Good idea to pull this function out, but you could have taken advantage of this to reduce the duplication of the string formatting code, which appears twice.
There was a problem hiding this comment.
Thankyou for pointing that out, I have removed teh duplication.
There was a problem hiding this comment.
Very nice, cleanly written implementation
implement-shell-tools/wc/wc.js
Outdated
| totalBytes += byteCount; | ||
|
|
||
| let output = ""; | ||
| if (options.lines) output += `${lineCount} `; |
There was a problem hiding this comment.
There is some duplication of your output functions here, is there any way you could reduce this?
There was a problem hiding this comment.
yes, I refactored the code with a helper function to format outpts.
LonMcGregor
added
Reviewed
SaraTahir28
added
the
Needs Review
|
Excellent work |
LonMcGregor
removed
Needs Review
LonMcGregor
added
the
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Implemented custom versions of wc, ls, and cat with core flags and behaviors to match standard Unix tools. Fixed BSD/macOS cat discrepancy,macOS cat (BSD) resets numbering per file with -n, but my Node.js implementation follows GNU cat behavior, continuing numbering across multiple files.