-
-
Notifications
You must be signed in to change notification settings - Fork 42
Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 3 | Implement Shell Tools #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 3 | Implement Shell Tools #248
Conversation
…essage and exit the program (unsuccessful).
…ption to handle the number of output lines
…he for commander library
… either a file or a dir
MattGoodwin0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've got a good understanding of wc, ls and cat commands.
Take a look at the layout of your code files to ensure a consistent format - using a standard format will make it easier to read and review your code.
Could you add some wording around the advantage of having separate package.json files?
| let allContent = directoryContent; | ||
|
|
||
| if (!options.a) { | ||
| allContent = directoryContent.filter(name => !name.startsWith(".")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if a directory was not provided - are we able to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matt, apologies for the very delayed response!
Thanks for bringing this to my attention.
I've just added validation to handle such a case.
Please review and let me know.
Regarding "Could you add some wording around the advantage of having separate package.json files?", was this a question in a backlog? I could not locate it if that was the case. |
Not so much a question in the backlog, we would usually expect one package.json, rather than three. Feel free to message on Slack to discuss |
My bad! Now I understand. Indeed, it does not make any sense having all 3 of them. Perhaps at the time I presumed that was needed. I will get rid of the extra 2 and ensured that the one package.json remaining is positioned correctly so all 3 folders (cat, ls & wc) have access to it without issues. If for whatever reason this does not work, I will ping you in Slack. Thanks Matt :) |
…ccess rather than each one having their own package.json
2. get rid of package-lock.json since a new one can be generated
… running npm install
|
Updated as recommended. Hope it looks better now. Thank you :) |
Self checklist
This PR provides implementations of shell tools: cat, ls, and wc, using JavaScript with Node.js.
Questions
Is the point of implementing those shell command as a way to appreciate the connivance they provide?
Are there any special cases where implementing shell commands might be needed?
Update: Having just stated Sprint 4, I now know that Shell command tools may not provide a full resolution to our problem. So to me, this is an example of when we could resort to building our own custom tools.