ZA | 25-SDC-July | Luke Manyamazi | Sprint 4 | Python Implement Shell Tools Exercises by Luke-Manyamazi · Pull Request #148 · CodeYourFuture/Module-Tools

Luke-Manyamazi · 2025-08-05T14:03:46Z

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

This PR implements basic versions of the following Unix commands in Python:

cat: Concatenates and displays file contents, with support for -n (number all lines) and -b (number non-empty lines).
ls: Lists directory contents, supporting options for showing hidden files and listing in one column.
wc: Counts lines, words, and bytes in a file, with flags to show specific counts.

These scripts were designed to mimic standard command-line tools and handle typical edge cases like missing files and glob patterns.

Questions

Are there any improvements you would suggest for handling edge cases (e.g., invalid file paths, empty files)?
Is the use of argparse and glob in each script appropriate and idiomatic?
Would you recommend any refactoring for readability or modularity?
Are there any Python best practices I may have missed?

illicitonion

This is generally looking good, but I left a few things to think about :)

illicitonion · 2025-08-06T17:34:19Z

+import glob
+import argparse
+
+def cat(filepath, n=False, b=False, line_counter=None):


n and b are both:

Not very expressive names

Seem mutually exclusive - it doesn't make sense for someone to pass both.

Can you think of a way of passing this information into the function which is easier to read, and makes more clear that only one of these should be set?

illicitonion · 2025-08-06T17:36:11Z

+                    else:
+                        print(line, end='')
+                elif n:
+                    print(f"{line_counter[0]:6}\t{line}", end='')


There's some repetition here between the b case and n case - imagine we wanted to start padding the line number by 8 instead of 6 characters - we'd need to change that in two places. Can you think how to avoid that?

We can create a single helper function for printing a numbered line. This avoids duplicating the f"{line_counter[0]:6}" formatting. Then in cat(), both the “all lines” and “non-empty lines” cases can just call this helper.

illicitonion · 2025-08-06T17:36:56Z

+
+    files = []
+    for pattern in args.files:
+        files.extend(glob.glob(pattern) or [pattern])


It's interesting you're globbing here - I would expect a shell to handle this. What would break if you removed the glob expansion yourself?

illicitonion · 2025-08-06T17:53:00Z

+                if b:
+                    if line.strip():
+                        print(f"{line_counter[0]:6}\t{line}", end='')
+                        line_counter[0] += 1


Why is this value in an array? You always seem to look at exactly one value in the array?

illicitonion · 2025-08-06T17:54:37Z

+import glob
+import argparse
+
+def cat(filepath, n=False, b=False, line_counter=None):


It looks like you always pass values for these arguments - why are you supplying default values for them? Particularly for line_counter where if None is used your code will actually break.

illicitonion · 2025-08-06T17:55:30Z

+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")
+
+def main():


How did you test this implementation?

I created two files and tried using cat -n /file/1 /file/2 and cat -b /file/1 /file/2 and compared the output with using your script, and didn't always get the same results

This comment still stands - I get different results between your program and the builtin cat.

illicitonion · 2025-08-06T17:55:51Z

+import os
+import argparse
+
+def ls(path='.', one_column=False, show_hidden=False):


Same question as above about default args

illicitonion · 2025-08-06T17:58:25Z

+        if count_words: parts.append(str(len(words)))
+        if count_bytes: parts.append(str(bytes_))
+
+        if not parts:


This works, but it feels like it's testing for a side-effect ("We didn't add any paths") rather than the cause ("you didn't ask for any of lines, words, or bytes") - I'd maybe rewrite this if to be framed in what the user requested.

(But the code works fine as it is, too)

illicitonion · 2025-08-06T17:59:18Z

+    parser.add_argument('paths', nargs='+', help='Files to count')
+    args = parser.parse_args()
+
+    for path in args.paths:


If I pass multiple files, the real wc outlines a line that you don't - can you add that too?

…for all the files

Luke-Manyamazi · 2025-10-13T19:17:53Z

cat.py

Removed the mutable list [1] hack for line numbering; now cat() returns the updated line number.
Introduced a Numbering enum (NONE, ALL, NONEMPTY) to make it explicit which numbering mode is active.
Added a helper function print_numbered_line() to avoid repeating the line number formatting logic.
Enforced mutual exclusivity of -n and -b in the argument parser.
Keeps numbering consistent across multiple files.

ls.py
Removed default values for one_column and show_hidden in the function signature; main() now passes them explicitly.
Added handling for NotADirectoryError to mimic real ls behavior.
Retained sorting and optional hidden file filtering.

wc.py
Replaced the “side-effect” check (if not parts) with an explicit check of what the user requested (-l, -w, -c).
Returns counts from wc() to allow aggregation for multiple files.
Prints totals if multiple files are provided, mimicking standard wc output.
Handles file-not-found and directory errors gracefully.

Luke-Manyamazi · 2025-10-15T14:01:38Z

Hi @illicitonion, please kindly review and see if all the requirements were met. If so please mark the PR as complete. Thank you.

illicitonion

This is generally looking good, but there are a bunch of small details and consistencies to look at :)

illicitonion · 2025-10-15T14:26:44Z

+                else:
+                    print(line, end='')
+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")


Does this print to stdout or stderr? Which should it print to? Why?

illicitonion · 2025-10-15T14:27:03Z

+    except FileNotFoundError:
+        print(f"cat: {filepath}: No such file or directory")
+
+def main():


This comment still stands - I get different results between your program and the builtin cat.

illicitonion · 2025-10-15T14:28:43Z

+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")
+
+def main():


Same question about testing here - If I run ls -a I get different files listed than if I run python3 ls.py -a.

illicitonion · 2025-10-15T14:28:57Z

+        print(f"ls: cannot access '{path}': No such file or directory")
+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")


Same question about stdout vs stderr

illicitonion · 2025-10-15T14:30:19Z

+        print(f"ls: cannot access '{path}': No such file or directory")
+    except NotADirectoryError:
+        print(f"ls: cannot access '{path}': Not a directory")


Stretch goal: What happens if you run ls /some/file for a file not a directory? What does your program do?

illicitonion · 2025-10-15T14:51:51Z

+    parser.add_argument('paths', nargs='+', help='Files to count')
+    args = parser.parse_args()
+
+    total_lines = total_words = total_bytes = 0


It's generally pretty unusual to initialise multiple variables like this in one go, because it makes it harder to skim the code to see where a particular variable was set/initialised than initialising each on its own line.

illicitonion · 2025-10-15T14:52:35Z

+
+    if multiple_files:
+        parts = []
+        if args.l or not any([args.l, args.w, args.c]): parts.append(str(total_lines))


You're repeating this not any([args.l, args.w, args.c]) three times - can you think of a way to avoid that duplication?

illicitonion · 2025-10-15T14:53:32Z

+
+        # Determine what to show
+        if not any([count_lines, count_words, count_bytes]):
+            count_lines = count_words = count_bytes = True


Again, I'd generally avoid putting this all on one line

illicitonion · 2025-10-15T15:00:03Z

+
+        print(' '.join(parts), path)
+
+        return (len(lines) if count_lines else 0,


It's interesting that you're doing this filtering twice - you're checking whether we should output lines both inside this function and when printing the totals.

Personally, I would probably just always return the values here, and let the caller decide whether to show them.

illicitonion · 2025-10-15T15:07:26Z

+        if count_lines: parts.append(str(len(lines)))
+        if count_words: parts.append(str(len(words)))
+        if count_bytes: parts.append(str(bytes_))


It's a little unusual that two of these have len calls and the last doesn't - not a big deal, but a bit oddly inconsistent - I would maybe make these consistent.

Added error handling for file access issues and improved output formatting.

Luke-Manyamazi · 2026-06-18T11:39:04Z

Thanks for the review. I've addressed the remaining comments by:

Sending error messages to stderr in cat, ls and wc.
Updating ls to handle file paths and include "." and ".." when using -a.
Re-testing cat and ls behaviour against the corresponding Unix commands.
Simplifying variable initialisation and duplicated conditions in wc.
Returning full counts from wc() and letting the caller determine what to display.
Making count handling more consistent throughout wc.

Please let me know if there is anything else that should be adjusted.

illicitonion

Sorry for the huge delay in getting back to you on this. Hopefully one last set of comments, which are hopefully useful to think about!

illicitonion · 2026-07-13T14:55:11Z

+                if numbering == Numbering.NONEMPTY:
+                    if line.strip():
+                        line_number = print_numbered_line(line, line_number)
+                    else:
+                        print(line, end='')
+                elif numbering == Numbering.ALL:
+                    line_number = print_numbered_line(line, line_number)


Personally I would probably to make the if more general here and write this as:

Suggested change

if numbering == Numbering.NONEMPTY:

if line.strip():

line_number = print_numbered_line(line, line_number)

else:

print(line, end='')

elif numbering == Numbering.ALL:

line_number = print_numbered_line(line, line_number)

if numbering == Numbering.ALL or (numbering == Numbering.NONEMPTY and line.strip()):

line_number = print_numbered_line(line, line_number)

else:

print(line, end='')

This is because the reason we're picking the two identical behaviours is the same ("We should number this line"), so we ideally only want to write the behaviour once.

And once we've done this, because there's no repetition, I'd probably stop having print_numbered_line return a new number, and instead just:

Suggested change

if numbering == Numbering.NONEMPTY:

if line.strip():

line_number = print_numbered_line(line, line_number)

else:

print(line, end='')

elif numbering == Numbering.ALL:

line_number = print_numbered_line(line, line_number)

if numbering == Numbering.ALL or (numbering == Numbering.NONEMPTY and line.strip()):

print_numbered_line(line, line_number)

line_number += 1

else:

print(line, end='')

illicitonion · 2026-07-13T14:58:18Z

+        if one_column:
+            print(*files, sep='\n')
+        else:
+            print(*files)


This is fine, but an alternative you may want to consider is:

Suggested change

if one_column:

print(*files, sep='\n')

else:

print(*files)

separator = "\t"

if one_column:

separator = "\n"

print(*files, sep=separator)

This may be more or less clear, I'm not going to argue strongly one way or the other, but is in keeping with that general "highlight what's the same and what's different" idea.

illicitonion · 2026-07-13T15:00:22Z

+        return (0, 0, 0)
+
+    except IsADirectoryError:
+        print(


If this happens, what will the exit code of the process be? What should it be?

(Same question for FileNotFoundError)

illicitonion · 2026-07-13T15:02:05Z

+        if args.c or show_all:
+            parts.append(str(total_bytes))
+
+        print(' '.join(parts), 'total')


This is repetitive with the formatting code, but as it's currently structured it's quite fiddly to deduplicate. Here's a set of steps that may help:

Move the "if none of the flags are set, set them all to true" code into just after args = parser.parse_args. Actually update args.l and args.w and args.c as a result. Then you don't need the or show_all any more.

Extract a function def print_stats(args, lines, words, bytes, filename) which you can call from both places.

Luke-Manyamazi added 4 commits August 1, 2025 05:15

added python the code for the cat command

3a31fbf

added python code for the ls command

d443669

added a destination to handel the -1 as a string

1900a28

added python code for the wc exercise

ff86f10

Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 5, 2025

illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 6, 2025

illicitonion reviewed Aug 6, 2025

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 6, 2025

Luke-Manyamazi linked an issue Aug 7, 2025 that may be closed by this pull request

01 Implement shell tools (cat, ls, wc) in Python Luke-Manyamazi/My-Coursework-Planner#177

Open

Luke-Manyamazi force-pushed the python-shell-tools-exercises branch from 35a4349 to ff86f10 Compare August 14, 2025 11:09

replace -n/-b flags with Numbering Enum

fcac82c

Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2025

refactored and corrected code to meet and answer the asked questions …

5476f45

…for all the files

illicitonion reviewed Oct 15, 2025

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 2025

Luke-Manyamazi added 3 commits June 18, 2026 13:37

Refactor wc function for clarity and error handling

84dcfb7

Enhance ls function with error handling and formatting

d72fc0a

Added error handling for file access issues and improved output formatting.

Improve error handling and formatting in cat.py

575b3f9

Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 18, 2026

illicitonion reviewed Jul 13, 2026

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 13, 2026


		print(' '.join(parts), path)

		return (len(lines) if count_lines else 0,

Uh oh!

Uh oh!

Conversation

Luke-Manyamazi commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

illicitonion 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luke-Manyamazi commented Oct 13, 2025

Uh oh!

Luke-Manyamazi commented Oct 15, 2025

Uh oh!

illicitonion 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!

Choose a reason for hiding this comment

Uh oh!

Luke-Manyamazi commented Jun 18, 2026

Uh oh!

illicitonion 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Luke-Manyamazi commented Aug 5, 2025 •

edited

Loading