Skip to content

London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python - #1

Open
Droid-An wants to merge 11 commits into
mainfrom
implement-shell-tools-in-python
Open

London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python#1
Droid-An wants to merge 11 commits into
mainfrom
implement-shell-tools-in-python

Conversation

@Droid-An

@Droid-An Droid-An commented Feb 9, 2026

Copy link
Copy Markdown
Owner

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

Implemented shell tools in Python

Questions

what formatter to use for python?

Comment on lines +28 to +30
else:
print(str(line_num).rjust(6), lin)
line_num += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should numbered output maintain the same line structure as the original file?

else:
print(lin)
else:
print(f"cat: {path}: Is a directory")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the program exit on first directory error or continue processing other files?

Comment on lines +24 to +25
data_to_proceed = [f for f in files if not f.startswith('.')]
data_to_proceed.sort(key=str.lower)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should hidden files be sorted case-insensitively when -a is used?

for f in output:
print(f)
else:
print(" ".join(data_to_proceed))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we implement proper column formatting that adapts to terminal width?

Comment on lines +46 to +48
lin = line.rstrip()
wds = lin.split()
word_count += len(wds)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we count words exactly as wc does, including handling of tabs and multiple spaces?

output_for_one_file.append(word_count)

if args.c:
file_size = os.path.getsize(path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should byte count be calculated from file content rather than file size?

output.append(total_results)

num_cols = len(total_results) - 1
column_widths = [len(str(total_results[i])) for i in range(num_cols)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should column widths be calculated from the actual values rather than wrapped in lists?

import os

parser = argparse.ArgumentParser(
prog="cat",

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cat

@Droid-An Droid-An added Needs Review documentation Improvements or additions to documentation and removed Needs Review documentation Improvements or additions to documentation labels Jun 7, 2026
@droid-an-ai-code-reviewer-dev

Copy link
Copy Markdown

Thanks for opening a new PR! AI started to review it

Comment on lines +22 to +33
line_num = 1
for line in lines:
lin = line.rstrip("\n")
if args.b:
if lin == "":
print()
else:
print(str(line_num).rjust(6), lin)
line_num += 1
elif args.n:
print(str(line_num).rjust(6), "", lin)
line_num += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the branches for -b and -n you have very similar behavior around numbering and printing lines:

  • In the -b branch you right‑justify the line number and then increment line_num when you print a non‑blank line.
  • In the -n branch you also right‑justify and print the line number, then increment line_num.

The main difference is that -b skips blank lines for numbering, while -n numbers every line. When you see this kind of “same structure with a small behavioral difference”, it can be a good candidate to extract into a helper function or a small shared block. That can make it easier to tweak the formatting or numbering in one place later.

You might ask yourself:

  • Is there a natural way to separate "how to format and print a numbered line" from "when to increment the line number"?
  • Could a small helper for printing a numbered line (including the rjust(6) padding) reduce repetition and make the code a bit easier to read?

Thinking in terms of small reusable pieces often helps keep the main loop focused on the control flow rather than the formatting details.

To "like" or "dislike" this comment, please follow this link

Comment on lines +18 to +21
if args.a:
data_to_proceed = [f for f in files]
data_to_proceed.sort()
data_to_proceed.insert(0, "..")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of places where you create new lists that are essentially copies of existing ones:

  • In the -a branch: data_to_proceed = [f for f in files] just recreates the same list.
  • In the -1 branch: output = [f for f in data_to_proceed] creates another copy before immediately iterating to print.

This kind of duplication isn’t a functional bug, but it does add extra steps that don’t change behavior. When you notice yourself creating new lists that are used exactly like the original, it can be worth asking:

  • Do I actually need a separate copy here, or can I work directly with the original variable?
  • Is there any later modification that justifies the copy, or is it purely redundant?

Reducing these redundant copies can simplify the code and make it clearer which list is the “source of truth” at each point.

To "like" or "dislike" this comment, please follow this link

Comment on lines +26 to +29
if args.one:
output = [f for f in data_to_proceed]
for f in output:
print(f)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In arguments_proceeding, the output variable is created as a separate list but then only used immediately in the for loop to print each element:

  • output = [f for f in data_to_proceed]
  • for f in output: print(f)

Since output is just a shallow copy of data_to_proceed and isn’t modified independently, it doesn’t add extra clarity; it’s just one more name to keep track of. This is a good spot to ask yourself: does this extra variable help me understand the code better, or could I work directly with data_to_proceed without losing readability? If you can work directly with the existing list, you keep the function simpler and reduce mental overhead when reading or changing it later.

To "like" or "dislike" this comment, please follow this link

Comment on lines +15 to +31
def arguments_proceeding(files):
data_to_proceed = files

if args.a:
data_to_proceed = [f for f in files]
data_to_proceed.sort()
data_to_proceed.insert(0, "..")
data_to_proceed.insert(0, ".")
else:
data_to_proceed = [f for f in files if not f.startswith('.')]
data_to_proceed.sort(key=str.lower)
if args.one:
output = [f for f in data_to_proceed]
for f in output:
print(f)
else:
print(" ".join(data_to_proceed))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function names arguments_proceeding and path_proceeding and the variable name data_to_proceed are a bit misleading. The word "proceeding" usually refers to an event or process that is happening (or to a legal proceeding), not to the act of processing data. Someone reading this might expect these functions to be related to control flow or program execution order, rather than "processing arguments" or "processing a path" to produce output. Similarly, data_to_proceed doesn’t clearly say that this is the list of filenames that will be filtered and printed.

How might you rename these so that, if someone saw only the function/variable names in isolation, they could accurately guess what the code does (e.g., that you are taking an input list of filenames, optionally filtering it, sorting it, and then printing it)? Thinking about names that describe the role of the function ("what it does" and "to what") can make the code easier to follow for the next reader—and for future you.

To "like" or "dislike" this comment, please follow this link

Comment on lines +34 to +41
def path_proceeding(path_argument):
if os.path.isfile(path_argument):
print(path_argument)
elif os.path.isdir(path_argument):
files = os.listdir(path_argument)
arguments_proceeding(files)

path_proceeding(args.path) No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function path_proceeding has a name that doesn’t clearly communicate its behavior. From the name alone, a reader might not realize that it is responsible for handling both the "file" and "directory" cases and delegating to arguments_proceeding when given a directory. The verb "proceeding" again doesn’t suggest that this is primarily about inspecting a path and printing its contents.

If you imagine someone calling this function from somewhere else with no context, what name would make it clear that it either prints a filename or lists directory contents depending on the input? Choosing a more descriptive verb can make the control flow much easier to understand at a glance.

To "like" or "dislike" this comment, please follow this link

Comment on lines +5 to +7
prog="wc",
description="Counts words in a file that contain a particular character",
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argparse.ArgumentParser description string says: "Counts words in a file that contain a particular character". That description doesn’t match what the program actually does: the code counts lines, words, and bytes depending on the flags, and doesn’t use any "particular character" filter.

This mismatch can easily mislead anyone using the script or reading the top of the file to understand its purpose—they might expect a grep-like or filtered word counter instead of a wc-style tool. Would a description that mentions lines/words/bytes and the -l, -w, and -c options better align reader expectations with the real behavior of the script?

To "like" or "dislike" this comment, please follow this link

Comment on lines +38 to +41
# lines count
if args.l:
num_lines = len(lines)
output_for_one_file.append(num_lines)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this line you’ve added a comment # lines count just before num_lines = len(lines). Because the variable name (num_lines) and the code (len(lines)) already describe very clearly what’s happening, this comment doesn’t really add extra information. Sometimes comments like this can become noise and make it harder to spot the comments that truly matter (for example, explaining a tricky edge case or a non-obvious design choice).

You might ask yourself: if someone who knows Python reads num_lines = len(lines), would they already understand that this is the line count without the comment? If the answer is yes, perhaps the code is already self-explanatory and the comment can be removed.

A useful rule of thumb is: could I rename a variable or tweak the structure so that the intent is obvious without needing a comment? In this case, it looks like you already have that clarity in the code itself.

To "like" or "dislike" this comment, please follow this link

Comment on lines +42 to +51
# word count
if args.w:
word_count = 0
for line in lines:
lin = line.rstrip()
wds = lin.split()
word_count += len(wds)

output_for_one_file.append(word_count)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this comment # word count sits directly above code that clearly computes a word count: you initialize word_count, iterate over lines, split them into words, and sum the lengths. For a reader familiar with Python and basic text-processing, the purpose is already evident from the code and variable names, so the comment doesn’t add much new information.

When you see yourself writing a comment that just restates the code ("word count" over a block that obviously counts words), it can be helpful to pause and consider: is there anything non-obvious or surprising about this block that a comment should explain? If not, leaving the code to speak for itself often keeps things cleaner and makes truly important comments stand out.

Over time, you’ll get a feel for when comments are best used to explain why something is done in a particular way, rather than what the straightforward code is doing.

To "like" or "dislike" this comment, please follow this link

Comment on lines +71 to +74
line_parts = []
for i in range(num_cols):
# Right-align with 1 space padding (like wc)
line_parts.append(str(row[i]).rjust(column_widths[i] + 6))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment # Right-align with 1 space padding (like wc) is more descriptive, but notice that the code (rjust(column_widths[i] + 6)) doesn’t obviously match the “1 space padding” wording. If the exact formatting is important to mimic the real wc command, you might consider whether the code can be made to reflect that intention more directly (for example, by using a named constant or a formatting helper) so the comment becomes unnecessary or minimal.

You might ask yourself: could I encode the idea of “right-align like wc” into the structure of the code or into a clearly named variable instead of a prose comment? For instance, having something like PADDING = 6 with a short docstring or a clearer calculation can make the intent self-documenting. That way, if you later change the padding, you won’t risk the comment drifting out of sync with the implementation.

Comments that describe formatting details can also go stale as the formatting evolves, so relying on code structure and names where possible tends to be more robust.

To "like" or "dislike" this comment, please follow this link

@Droid-An Droid-An added documentation Improvements or additions to documentation bug Something isn't working duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed Needs Review and removed Needs Review labels Jun 8, 2026
@droid-an-ai-code-reviewer-dev

Copy link
Copy Markdown

Thanks for opening a new PR! AI started to review it

Comment on lines +18 to +23
for path in args.path:
if os.path.isfile(path):
with open(path) as file:
lines = file.readlines()
line_num = 1
for line in lines:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your current implementation, line_num is reset to 1 for every file you process (line_num = 1 inside the for path in args.path loop). On both macOS and Linux, cat -n and cat -b keep numbering across multiple files rather than restarting at each one. Because of the current placement of line_num, the numbering will restart at 1 for each new file, which changes the observable behavior.

Would it make sense to move the line_num variable so that it’s shared across all files instead of being recreated inside each file loop, so the numbering matches typical cat behavior?

Comment on lines +5 to +7
prog="wc",
description="Counts words in a file that contain a particular character",
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text in the ArgumentParser description says "Counts words in a file that contain a particular character", but the program actually counts lines, words, and bytes like the standard wc tool, and it doesn’t filter on any particular character. A future reader (or your future self) might reasonably expect some character-based filtering because of this wording and be confused when they don’t find it in the logic.

Would updating the description to match the real behavior of the script (counting lines/words/bytes) make it clearer what this tool does at a glance?

Comment on lines +67 to +69
elif len(args.path) == 1:
num_cols = len(output[0]) - 1
column_widths = [len(str([output[0][i]])) for i in range(num_cols)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the elif len(args.path) == 1: branch, column_widths is computed as [len(str([output[0][i]])) for i in range(num_cols)]. Because of the extra brackets, this is actually computing the length of the string representation of a list containing the value (e.g. '[12]' has length 4) instead of the value itself (e.g. '12' has length 2). This leads to wider-than-necessary columns and inconsistent formatting compared to the multi-file case above.

If you compare how column_widths is calculated in the multi-file branch with this single-file branch, what change to the expression here would make the behavior consistent in both cases?

Comment on lines +38 to +41
# lines count
if args.l:
num_lines = len(lines)
output_for_one_file.append(num_lines)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this line you add the comment # lines count just before num_lines = len(lines). Because num_lines = len(lines) is already very short and clearly expresses what is happening, the comment doesn’t really add extra information. Over time, comments that simply repeat what the code says can make the file noisier without helping future readers.

When you feel tempted to write a comment like this, it can be useful to ask: “If I renamed the variable or extracted a small helper, would the intent be obvious without a comment?” In this case, even something like line_count = len(lines) would likely be enough on its own.

How might you decide when a comment is actually clarifying some non-obvious behavior versus just restating the code in English? Developing that instinct will help you keep comments focused on the “why” instead of the “what.”

Comment on lines +42 to +51
# word count
if args.w:
word_count = 0
for line in lines:
lin = line.rstrip()
wds = lin.split()
word_count += len(wds)

output_for_one_file.append(word_count)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment # word count is sitting right above a straightforward block that initializes word_count = 0, loops over lines, splits them, and sums their lengths. The code already communicates that you are counting words, so the comment doesn’t add much. If later you changed the way words are counted (for example, to handle punctuation differently) there’s also a chance the comment could get out of sync with the behavior.

One way to think about this is: could you express this logic in a way that makes the intent obvious without a comment, for example by extracting a helper like count_words(lines) or choosing even more descriptive variable names? That kind of small refactor often makes comments like this unnecessary.

What information would you include in a comment here that isn’t already visible from the code itself (for example, any subtle rules about what you consider a “word”)? If there isn’t anything extra to say, it might be a sign the comment isn’t needed.

Comment on lines +73 to +74
# Right-align with 1 space padding (like wc)
line_parts.append(str(row[i]).rjust(column_widths[i] + 6))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment # Right-align with 1 space padding (like wc) explains the formatting logic before str(row[i]).rjust(column_widths[i] + 6). A future reader can probably infer the alignment from the rjust call itself, so part of the comment is just restating the code. The interesting part is the comparison to the system wc command.

When you write comments, it can be helpful to separate what the code does (which the code already shows) from why you chose to do it this way. Here, the “like wc” piece is about intent and could be valuable, but the “Right-align with 1 space padding” part is more or less encoded in the expression.

You might ask yourself: if someone saw only the rjust(column_widths[i] + 6) line, what might be confusing? Is it the specific constant 6, or the fact that you’re mimicking the behavior of another tool? If the magic number is the confusing part, could a well-named constant or helper make that clearer and reduce the need for an explanatory comment?

@Droid-An Droid-An added bug Something isn't working and removed bug Something isn't working documentation Improvements or additions to documentation duplicate This issue or pull request already exists enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers Needs Review labels Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant