Cape Town| 25-SDC-July | Faith Muzondo | Sprint 3| Implement shell tools#128
Cape Town| 25-SDC-July | Faith Muzondo | Sprint 3| Implement shell tools#128Faithy4444 wants to merge 9 commits intoCodeYourFuture:mainfrom
Conversation
Faithy4444
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 on this task. There are a few issues still to resolve, though
implement-shell-tools/cat/cat.mjs
Outdated
| program.help(); | ||
| } | ||
|
|
||
| const path = args[0] |
There was a problem hiding this comment.
Your cat implementation seems to struggle if I try to pass more than one file at a time, e.g. using a * wildcard. Can you find a way to make it work if multiple are passed in?
There was a problem hiding this comment.
i have made changes to make it work for multiple files
implement-shell-tools/ls/ls.mjs
Outdated
| } | ||
|
|
||
| if (opts.one) { | ||
| visibleFiles.forEach(file => console.log(file)); |
There was a problem hiding this comment.
as you are passing an anonymous function which is passing an argument into a single function here, do you know of any way this call could be simplified?
implement-shell-tools/wc/wc.mjs
Outdated
| program.help(); | ||
| } | ||
|
|
||
| const path = args[0]; |
There was a problem hiding this comment.
In the original wc program I can pass multiple files in to get a different kind of output. Can you see why your program doesn't match this behaviour?
implement-shell-tools/wc/wc.mjs
Outdated
| const words = countWords(content); | ||
| const bytes = countBytes(content); | ||
|
|
||
| if (opts.line) { |
There was a problem hiding this comment.
In the original wc program I can combine different combinations of option. For example, to get line and word but not character counts. Can you spot the issue with your implementation?
LonMcGregor
added
Reviewed
|
This looks good |
LonMcGregor
added
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). |
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). |
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.