London | 25-SDC-Nov | Aida Eslamimoghadam | Sprint 3 | Implement Shell Tools#249
London | 25-SDC-Nov | Aida Eslamimoghadam | Sprint 3 | Implement Shell Tools#249aydaeslami wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
aydaeslami
added
📅 Sprint 3
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on these tasks, i've left some pointers.
Also, as a general note, we try to be consistent in how we structure our projects and files. Can you spot the difference between how you've named your files in cat, ls, and wc?
There was a problem hiding this comment.
Good solution, but consider how the numbers will display if you had longer files. Is there some formatting you could add to improve it?
| continue; | ||
| } | ||
|
|
||
| if (opts["1"]) { |
| const content = await fs.readFile(file, "utf-8"); | ||
|
|
||
| totalChars += content.trim().length; | ||
| // const charList = content.trim().split(/\s+/); |
There was a problem hiding this comment.
Remember we try to avoid comitting commented out code that is no longer needed
There was a problem hiding this comment.
This solution works, but it is a bit non-standard compared to the original wc implementation.
Notably, this utility is just about getting statistics, so you don't need to output the files content. That is what cat is for. Could you take another look at this one?
LonMcGregor
added
Reviewed
eliza-an
left a comment
eliza-an
left a comment
There was a problem hiding this comment.
good start, just a few notes as I mentioned. Leon added some as well as i saw!
| lineNumber++; | ||
| } | ||
| } else if (opts.b) { | ||
| const lines = content.split("\n"); |
There was a problem hiding this comment.
this can be exported to the outer loop as this line repetition is unnecessary ( line 22 also)
| @@ -0,0 +1,40 @@ | |||
| import { program } from "commander"; | |||
| import { promises as fs } from "node:fs"; | |||
| import process from "node:process"; | |||
There was a problem hiding this comment.
i don't think this import is ever used
| @@ -0,0 +1,36 @@ | |||
| import { program } from "commander"; | |||
| import { promises as fs } from "node:fs"; | |||
| import process from "node:process"; | |||
| .description("my ls clone") | ||
| .option("-1", "one entry per line") | ||
| .option("-a", "show hidden files") | ||
| .argument("[paths...]", "file or directory paths"); |
There was a problem hiding this comment.
good use of an optional parameter here!
| } | ||
|
|
||
| for (const path of paths) { | ||
| const entries = await fs.readdir(path); |
There was a problem hiding this comment.
Not a huge fan of the name entreis here. Would one of your peers marking this know exactly what you are talkign about? Try to use a bit clearer naming patters :)
| @@ -0,0 +1,68 @@ | |||
| import { program } from "commander"; | |||
| import { promises as fs } from "node:fs"; | |||
| import process from "node:process"; | |||
There was a problem hiding this comment.
also unnecessary import here. Can you have a google why?
|
|
||
| program | ||
| .name("myLs") | ||
| .description("my ls clone") |
3 participants
Changelist
This PR includes my solutions to simulate several shell tools.
I used the Commander module to re-implement them.