-
-
Notifications
You must be signed in to change notification settings - Fork 92
London | 26-SDC-MAR | Jamal Laqdiem | Sprint 3 | Implement Shell Tools #437
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?
Changes from 3 commits
276fc2b
8675a8b
99cb233
e1d7c80
2bec857
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import process from "node:process"; | ||
| import { promises as fs } from "node:fs"; | ||
|
|
||
| // THis will give an array without the path to node and to the file. | ||
| const argv = process.argv.slice(2); | ||
|
|
||
| //Get line numbers. | ||
| const showNumbers = argv.includes("-n"); | ||
| const showNonBlankNumbers = argv.includes("-b"); | ||
|
|
||
| //filter the - from the array argv as it's a flag. | ||
| const filePaths = argv.filter((arg) => !arg.startsWith("-")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if someone passed a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, you are right to point that out, my logic will let the user confused thinking that the -q flag exists and worked, I implemented in place a flag check and ensured print an error and exit with not 0 code. |
||
| let counterLines = 1; | ||
|
|
||
| for (const path of filePaths) { | ||
| try { | ||
| const content = await fs.readFile(path, "utf-8"); | ||
|
|
||
| //split the text at the new line character. | ||
| const splitLines = content.split("\n"); | ||
|
|
||
| splitLines.forEach((line) => { | ||
| if (showNumbers) { | ||
| console.log(`${counterLines++} ${line}`); | ||
| } else if (showNonBlankNumbers) { | ||
| // increment and show numbers only if the line is not empty. | ||
| if (line.trim() !== "") { | ||
| console.log(`${counterLines++} ${line}`); | ||
| } else { | ||
| // print empty lines | ||
| console.log(line); | ||
| } | ||
| } else { | ||
| console.log(line); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three branches here look quite similar and repetitive. In general, if you have multiple similar branches, it's more clear to extract the differences into variables, and then run the same code, i.e. so you'd only have one call to This way it's easier for someone reading the code to see what's the same / different in each case, and also avoids the hazard that someone updates one of the branches but forgets to update the other ones.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, I followed your correct instructions , as it's much clearer and readable using a variable. |
||
| } | ||
|
Comment on lines
+31
to
+37
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think about collapsing this into one if: if (showNumbers || (howNonBlankNumbers && line.trim() !== "")) {because they have the same intent, but wouldn't push strongly for this if you prefer it as-is :) |
||
| }); | ||
| } catch (error) { | ||
| console.log(`Could not read: ${path}`); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's an error where will this message get printed? stdout or stderr? And what exit code would the process exit with? Do these match our expectations from shell tools?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, I made the changes from console.log that wil send txt to stdout To console.error that willsend to stderr and display the error in the screen. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||
| import fs from "node:fs"; | ||||||
| import process from "node:process"; | ||||||
|
|
||||||
| // This will give an array without the path to node and to the file. | ||||||
| const argv = process.argv.slice(2); | ||||||
|
|
||||||
| // filter the flag to find the target folder. | ||||||
| const filePaths = argv.filter((arg) => !arg.startsWith("-")); | ||||||
| const showHiddenFiles = argv.includes("-a"); | ||||||
|
|
||||||
| // if no folder provide we use the current one | ||||||
| const target = filePaths[0] || "."; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if someone specified multiple paths, e.g. The README.md only requires that your programme works with simple paths, but I would recommend implementing support for multiple. But if you don't implement that, you generally want to give an error to the user if they supply input you don't expect, rather than just ignoring it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, I should at least display an stderr and exit . |
||||||
| // read the file. | ||||||
| const files = fs.readdirSync(target); | ||||||
|
|
||||||
| // save the result into the variable. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost all of your comments provide no value over just reading the code. Comments should explain why we're doing something (if it's not obvious), or explain something particularly confusing/tricky. If you can read the code, the comments here aren't useful - please consider all of them and remove any you think aren't useful. |
||||||
| let filteredFIles = files; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| if (!showHiddenFiles) { | ||||||
| filteredFIles = files.filter((file) => !file.startsWith(".")); | ||||||
| } else { | ||||||
| // we use spread operator to merge the paths. | ||||||
| filteredFIles = [".", "..", ...files]; | ||||||
| } | ||||||
|
|
||||||
| // Print using -1 . | ||||||
| filteredFIles.forEach((file) => { | ||||||
| console.log(file); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import process, { exit } from "node:process"; | ||
| import fs from "node:fs"; | ||
|
|
||
| const argv = process.argv.slice(2); | ||
|
|
||
| const showWords = argv.includes("-w"); | ||
| const showLines = argv.includes("-l"); | ||
| const showBytes = argv.includes("-c"); | ||
| const showCharacters = argv.includes("-m"); | ||
|
|
||
| // filter flags, and getting the string of filename | ||
| const filePaths = argv.filter((arg) => !arg.startsWith("-")); | ||
|
|
||
| const noFlags = !showLines && !showCharacters && !showWords && !showCharacters; | ||
| if (!filePaths) { | ||
| console.error("PLease provide a file path"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You frequently have typos where the second letter after a capital letter is also capitalised. Please look out for these and fix them. |
||
| process.exit(1); | ||
| } | ||
| // loop trough the array to get each file path. | ||
| filePaths.forEach((filePath) => { | ||
| const content = fs.readFileSync(filePath, "utf-8"); | ||
|
|
||
| const lines = content.split("\n").length - 1; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you have to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, I used -1 because my vscode automatically add a new line when save, so in this case split method will add an empty element, however in other environments this logic my fail if no trialling new line added .
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may add a conditional check using .pop method in case of empty string at the end
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do the conditional pop check :) |
||
| const words = content | ||
| .trim() | ||
| .split(/\s+/) | ||
| .filter((word) => word != "").length; | ||
| // here I used Buffer.byteLength even if characters and bytes can be the same number .length, however sometimes an emoji or special characters can be heavier 2b or 4b | ||
| const bytes = Buffer.byteLength(content); | ||
| const characters = content.length; | ||
|
|
||
| let output = ""; | ||
|
|
||
| if (showLines || noFlags) output += `${lines} `; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you think of a way to achieve this same result, but without needing a If our program was bigger and more complicated, in most of the program we don't want to have to think about what the exact flags interface was. Ideally we can just look at
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing, I implemented a clear way to switch all flags to true if no flag was picked. |
||
| if (showWords || noFlags) output += `${words} `; | ||
| if (showBytes || noFlags) output += `${bytes} `; | ||
| if (showCharacters || noFlags) output += `${characters} `; | ||
|
|
||
| console.log(`${output} ${filePath}`); | ||
| }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your output doesn't match real |
||
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.
Does this comment provide value beyond the variable names?
If yes, could you change the variable names to make this comment no longer necessary?