-
-
Notifications
You must be signed in to change notification settings - Fork 42
Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 3 |Implement shell tools #241
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 | Sheida Shabankari | Sprint 3 |Implement shell tools #241
Conversation
LonMcGregor
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.
Good work on these tasks, apologies for the lateness in review
You have taken a good approach at manually parsing the arguments. You might consider investigating if there are any libraries or APIs you could use to help you with this. In a more complex shell tool, it might be difficult to manually check arguments this way. It would also make it easier to combine arguments, e.g.allowing wc -w -l instead of only one at a time
implement-shell-tools/cat/cat.js
Outdated
| lineNumber ++ ; | ||
| } | ||
| } | ||
| } |
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.
The formatting is a bit unclear here, do you know of any tools in the command line or the IDE that could help to improve 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.
I’m using VS Code with Prettier enabled, but I’m not sure why the file wasn’t properly formatted when saved. I manually reformatted it afterwards.
implement-shell-tools/cat/cat.js
Outdated
| const filesNameArray = await glob(fileNamePattern); | ||
| filesNameArray.sort(); | ||
| let lineNumber = 1; | ||
| //console.log(filesNameArray); |
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.
Tip: Avoid committing commented old code
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.
I Fixed it.
| @@ -0,0 +1,46 @@ | |||
| import { promises as fs } from "node:fs"; | |||
| import { glob } from "glob"; | |||
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.
When I run this code I get an error Cannot find package 'glob'. Do you see this also?
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.
Thanks for your comment but I don't see any error !
implement-shell-tools/ls/ls.js
Outdated
| import { promises as fs } from "node:fs"; | ||
| async function listDir() { | ||
| let finalFileList; | ||
| const flag = process.argv[2]; |
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.
The readme also asks you to try implementing the -1 option, can you try 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.
When I initially implemented the code, the -1 flag behavior was applied automatically by default, so I didn’t explicitly implement it. However, based on your feedback, I have now updated the implementation to handle the -1 flag explicitly.
implement-shell-tools/wc/wc.js
Outdated
|
|
||
| for(const file of commandLineArray){ | ||
| const fileContent=await fs.readFile(file , 'utf-8'); | ||
| const fileBuffer=await fs.readFile(file); |
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.
Do you need to read the file in two times? What impact will this have on performance?
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're right — the file is being read twice unnecessarily, which affects performance. This could be improved by reading the file once and reusing the buffer. I updated the code.
implement-shell-tools/wc/wc.js
Outdated
| if(commandLineArray.length>1){ | ||
| switch (flags[0]) { | ||
| case "-l": | ||
| console.log(`${String(totalLines).padStart(4)} total`); |
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.
There is some duplicated code here, can you reduce that?
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.
I fixed it by implementing a function.
|
Good work on this. |
9138dac to
5b9cc2d
Compare
|
Thanks for your feedback.I think I fixed the issue. |
|
This looks good, good work |
Self checklist