London | 25-SDC-July | Franklin Kamela | Sprint 3 | Feature/Implement Shell Tools by Fradoka · Pull Request #135 · CodeYourFuture/Module-Tools

Fradoka · 2025-07-31T17:36:47Z

Learners, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
I have tested my changes
My changes follow the style guide
My changes meet the requirements of this task

Changelist

Questions

LonMcGregor

Hi,

Remember to update the template when you open a new PR on github - you have left the default text there, so I don't know what the goal of this PR is.

You have made a good start on these tasks. I've left some comments for how you could improve them further.

LonMcGregor · 2025-08-13T14:49:59Z

implement-shell-tools/cat/my_cat.js

+    showLineNumb = false;
+    showNoBlankNumb = true;
+  }else {  
+    file = arg; 


What happens if I want to pass multiple files in here? For example, using the * wildcard which expands to multiple files in the shell.

Your solution in wc seems a better approach

This seems to be fixed now - well done

LonMcGregor · 2025-08-13T14:51:15Z

implement-shell-tools/cat/my_cat.js

+    }).join('\n');
+  }
+
+  console.log (content)


Be careful of formatting your code the same way. Can you see why this line is inconsistent with the rest of the file?

LonMcGregor · 2025-08-13T14:52:56Z

implement-shell-tools/ls/my_ls.js

+args.forEach(arg => {
+  if (arg === '-a') {
+    showAll = true;
+  } else if (arg !== '-1') {


This approach might cause problems in two cases:

What if the user does not want each output per line and wants it all on one line (like the original ls) app?

Assuming anything else is a directory might make it more difficult to add new options in the future.

This is fixed now - great

LonMcGregor · 2025-08-13T14:55:13Z

implement-shell-tools/wc/my_wc.js

+    const totalBytes = validCounts.reduce((sum, c) => sum + c.bytes, 0);
+
+    let totalOutput = '';
+    if (showLines) totalOutput += `${totalLines} `;


There is some duplication here when you are printing totals and when you print single file output. Can you think of how to reduce this?

LonMcGregor · 2025-08-13T14:56:28Z

implement-shell-tools/wc/my_wc.js

+}
+
+async function main() {
+  const counts = await Promise.all(files.map(countFile));


This is a very minor point, but when doing map we expect it to return a new array that is different from the input array. Instead of .map can you think of another array function that would make more sense to use here?

Good solution

Fradoka · 2025-08-31T20:58:04Z

hi @LonMcGregor, i have now amended this cat, ls, and wc to address review.

github-actions · 2025-08-31T20:58:45Z

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

github-actions · 2025-08-31T20:58:49Z

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

LonMcGregor

Good work on updating these. The only thing left to work in is making sure that your cat can handle numbering multiple files

LonMcGregor · 2025-09-01T08:40:45Z

implement-shell-tools/cat/my_cat.js

+    showLineNumb = false;
+    showNoBlankNumb = true;
+  }else {  
+    file = arg; 


This seems to be fixed now - well done

LonMcGregor · 2025-09-01T08:42:03Z

implement-shell-tools/cat/my_cat.js

+    const lines = content.split('\n');
+
+    if (showNoBlankNumb) {     // Number only non-blank lines
+        let counter = 1;


Try testing your updated solution with the numbering options on with multiple files now your app supports it. How would you expect it to work? If you compare your solution to the original cat, can you see a discrepancy?

LonMcGregor · 2025-09-01T08:44:05Z

implement-shell-tools/ls/my_ls.js

+args.forEach(arg => {
+  if (arg === '-a') {
+    showAll = true;
+  } else if (arg !== '-1') {


This is fixed now - great

LonMcGregor · 2025-09-01T08:46:10Z

implement-shell-tools/wc/my_wc.js

+}
+
+async function main() {
+  const counts = await Promise.all(files.map(countFile));


Good solution

LonMcGregor · 2025-09-01T08:46:15Z

implement-shell-tools/wc/my_wc.js

+    const totalBytes = validCounts.reduce((sum, c) => sum + c.bytes, 0);
+
+    let totalOutput = '';
+    if (showLines) totalOutput += `${totalLines} `;


github-actions · 2025-09-01T08:47:07Z

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

github-actions · 2025-09-01T08:47:08Z

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

Fradoka · 2025-09-01T16:58:30Z

hi @LonMcGregor, amended my_cat.js to address review.

LonMcGregor · 2025-09-02T08:20:49Z

In my_cat.js why do you use two different counter variables? Is there a circumstance where you can be numbering both blank and non-blank lines at once where you would need to count these separately?

Fradoka · 2025-09-02T13:23:47Z

hi @LonMcGregor, i don't actually need 2 different counter variables as -b will overide -n so there won't be a time when i would need both set of numbering. i used 2 counters before to differentiate but a unique counter still does the job. i have now amended the code to address that.

LonMcGregor · 2025-09-02T13:31:11Z

@Fradoka It is worth considering the trade-offs here. Two counters would let you differentiate it, and maybe in a more complex app you might want to add features that require it, so it's good you considered this. But here it probably isn't necessary. Good work on this sprint task, it's done now!

Fradoka added 10 commits July 15, 2025 06:06

exercise ls

0611383

exercise cat

1f5b567

exercise wc

0a71087

exercise grep

72271cf

exercise sed

0d3bc17

exercise awk

22a04bb

exercise number systems

32a3a87

Revert mistaken commits from main branch

4ef88e2

implemented ls

4e42980

implemented cat and wc

647f171

Fradoka added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 31, 2025

LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 13, 2025

LonMcGregor reviewed Aug 13, 2025

View reviewed changes

Fradoka added 3 commits August 31, 2025 20:23

updated my_cat.js

e1a0f74

updated my_ls.js

a83f327

updated my_wc.js

7f7d707

Fradoka added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Aug 31, 2025

LonMcGregor reviewed Sep 1, 2025

View reviewed changes

LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 1, 2025

updated my_cat.js to address numbering for multiple files

a38f2e9

Fradoka added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 1, 2025

LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 2, 2025

amended my_cat.js to address review on counter variable

6098359

Fradoka added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 2, 2025

LonMcGregor added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 2, 2025

Uh oh!

Conversation

Fradoka commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

LonMcGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fradoka commented Aug 31, 2025

Uh oh!

github-actions bot commented Aug 31, 2025

Uh oh!

github-actions bot commented Aug 31, 2025

Uh oh!

LonMcGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 1, 2025

Uh oh!

github-actions bot commented Sep 1, 2025

Uh oh!

Fradoka commented Sep 1, 2025

Uh oh!

LonMcGregor commented Sep 2, 2025

Uh oh!

Fradoka commented Sep 2, 2025

Uh oh!

LonMcGregor commented Sep 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fradoka commented Jul 31, 2025 •

edited

Loading