LONDON | DONARA BLANC | MODULE TOOLS | SPRINT -3 #58
Conversation
donarbl
added
Needs Review
There was a problem hiding this comment.
Thank you for your PR! I'm Borja, a volunteer, and I'll be reviewing your code.
The implementations of cat and ls are good! I have left a few edge cases that are not covered, as well as a few suggestions, but you're definitely on the right track :)
The implementation of wc is missing a few parts:
- How could we handle being given several files? Like this case given in the problem description:
wc sample-files/* - How could we handle the following flags:
-l,-c, and-w?
Lastly, do we need the .DS_Store files?
Keep up the good work!
| function printLines(lines) { | ||
| lines.forEach((line) => { | ||
| if (bFlag && line.trim()) { | ||
| console.log(`${nonEmptyLineNum++} ${line}`); |
There was a problem hiding this comment.
There seems to be something wrong with the output. When I run this code, the lines on multiple files are counted the with the same counter:
$ node cat.js -b sample-files/*.txt
1 Once upon a time...
2 There was a house made of gingerbread.
3 It looked delicious.
4 I was tempted to take a bite of it.
5 But this seemed like a bad idea...
6 There's more to come, though...
However, when we run cat, we can see that every line number is counted individually:
➜ cat -b sample-files/*.txt
1 Once upon a time... # From sample-files/1.txt
1 There was a house made of gingerbread. # From sample-files/2.txt
1 It looked delicious. # From sample-files/3.txt
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4 There's more to come, though...How can we modify the code so that the lines are counted correctly?
| @@ -0,0 +1,45 @@ | |||
|
|
|||
| const { promises: fs } = require("fs");// it reads the files | |||
| const process = require("process");// gets large files information | |||
There was a problem hiding this comment.
Is this comment accurate? The only use of process I can find is process.argv, which doesn't match the comment.
|
|
||
| const { promises: fs } = require("fs");// it reads the files | ||
| const process = require("process");// gets large files information | ||
| const { program } = require("commander");// it puts out in chunks or parses information |
There was a problem hiding this comment.
Is this comment accurate? Is there a way you could shape the program that would codify this intention in the code (e.g. with a function name or a variable name), instead of relying on a comment?
| let lineNum = 1; // Counting for -n flag | ||
| let nonEmptyLineNum = 1; // Counting for -b flag |
There was a problem hiding this comment.
Comments can get out of date. Is there a way you can codify this intention in the code? e.g. via variable name.
| } | ||
| } catch (err) { | ||
| // Enhanced error handling to print ls behavior | ||
| console.error(`ls: cannot access '${dir}': No such file or directory`); |
There was a problem hiding this comment.
There might be other reasons why you can't access the directory. For instance, you may not have read access to it (For instance, if you get this output: Permission error: EACCES: permission denied, scandir ‘/restricted’).
How should we handle those cases?
| program | ||
| .name("ls") | ||
| .description("An implementation of the ls command") | ||
| .option("-a, --all", "Include hidden files") |
There was a problem hiding this comment.
When -a is set, it should also print the current directory (.) and the parent diractory (..).
➜ ls -1 -a sample-files
.
..
.hidden.txt
1.txt
2.txt
3.txt
dirHow could we modify this solution so that we do print them?
| .option("-a, --all", "Include hidden files") | ||
| .option("-1", "List one file per line") // Support for -1 flag | ||
| .argument("[directory]", "The directory to list", ".") // Default directory is "." | ||
| .parse(process.argv); |
There was a problem hiding this comment.
What happens when we receive more than one argument? (e.g. node ls.js directory1 directory2)
| files.forEach(file => console.log(file)); | ||
| } else { | ||
| // Default behavior: print files space-separated (like ls without -1) | ||
| console.log(files.join(" ")); |
There was a problem hiding this comment.
In ls, files are printed with a certain amount of padding, so that if we have many files, they appear to be in a table:
➜ /bin/ls
ls.js package-lock.json README.md
node_modules package.json sample-files
This is not part of the assignment brief but, as a streacth goal, would you like to consider how to pad the outputs like that?
blorente
added
👀 Review Git
Self checklist
Changelist
I've used libraries and implemented functions that work with cat, ls and wc and associated flags.
Questions
Ask any questions you have for your reviewer.