London | 25-SDC-July | Fatma Arslantas | Sprint 3 | Implement Shell Tools #133
Conversation
- Added "type": "module" to package.json - Installed commander as a dependency
AFatmaa
added
📅 Sprint 3
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Hi,
First, I notice you haven't updated the template in your pull request. when opening a new PR on github, remember to add in your own explanation in the text.
You have made a good start implementing the tasks. I have left some comments to point you towards how you could improve the more.
implement-shell-tools/cat/cat.js
Outdated
|
|
||
| for (const line of content.split("\n")) { | ||
| if (options.n) { | ||
| console.log(`${lineNumber} ${line}`); |
There was a problem hiding this comment.
Can you think of a way to ensure that once a number increased by a factor of 10, it doesn't bump the text along? (i.e. 10 takes up 1 more character space than 9)
There was a problem hiding this comment.
Thank you for the feedback and your time. You're right, numbers like '10' took up more space and pushed the text. I've fixed this by using padStart, and I updated my code.
| program | ||
| .name("my-ls") | ||
| .description("Reimplementation of the Unix `ls` command with -1 and -a options") | ||
| .option("-1", "list one file per line") |
There was a problem hiding this comment.
You have an option allowing one line per file. What does the user do if they dont want one line per file?
There was a problem hiding this comment.
Thanks for the feedback! You're right. I updated the code so that if -1 is not passed, the files will now be shown in a single line, just like the default ls command.
implement-shell-tools/wc/wc.js
Outdated
| let totalOutput = ""; | ||
|
|
||
| if (options.l || options.w || options.c) { | ||
| if (options.l) totalOutput += `${totalLines} `; |
There was a problem hiding this comment.
There is a lot of duplicated code when you are printing out individual and total lines. Can you think how to reduce this?
There was a problem hiding this comment.
Thank you for the feedback! I extracted the output logic into a separate function to remove duplication. Let me know what you think 🙏
LonMcGregor
added
Reviewed
|
Hi @LonMcGregor, |
|
Great work - thanks for making these changes. You are now done with this sprint task. You can close this now. A tip - if you respond to my review and want code reviewed again, remember to add the "needs review" label back onto this PR. |
LonMcGregor
added
Complete
|
Thanks a lot for your review and the helpful tips, @LonMcGregor 🌸 I’ll make sure to add the needs review label next time when I want another review. |
2 participants
Learners, PR Template
Self checklist
Changelist
This pull request implements the
cat,wc, andlsshell tools in JavaScript using NodeJS, as per the coursework instructions.I tried to make them work like the real tools for the tasks given in the README.md files.
Questions
Ask any questions you have for your reviewer.