London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python - #1
London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python#1Droid-An wants to merge 11 commits into
Conversation
| else: | ||
| print(str(line_num).rjust(6), lin) | ||
| line_num += 1 |
There was a problem hiding this comment.
Should numbered output maintain the same line structure as the original file?
| else: | ||
| print(lin) | ||
| else: | ||
| print(f"cat: {path}: Is a directory") |
There was a problem hiding this comment.
Should the program exit on first directory error or continue processing other files?
| data_to_proceed = [f for f in files if not f.startswith('.')] | ||
| data_to_proceed.sort(key=str.lower) |
There was a problem hiding this comment.
Should hidden files be sorted case-insensitively when -a is used?
| for f in output: | ||
| print(f) | ||
| else: | ||
| print(" ".join(data_to_proceed)) |
There was a problem hiding this comment.
Should we implement proper column formatting that adapts to terminal width?
| lin = line.rstrip() | ||
| wds = lin.split() | ||
| word_count += len(wds) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
Should column widths be calculated from the actual values rather than wrapped in lists?
| import os | ||
|
|
||
| parser = argparse.ArgumentParser( | ||
| prog="cat", |
|
Thanks for opening a new PR! AI started to review it |
| 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 |
There was a problem hiding this comment.
In the branches for -b and -n you have very similar behavior around numbering and printing lines:
- In the
-bbranch you right‑justify the line number and then incrementline_numwhen you print a non‑blank line. - In the
-nbranch you also right‑justify and print the line number, then incrementline_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
| if args.a: | ||
| data_to_proceed = [f for f in files] | ||
| data_to_proceed.sort() | ||
| data_to_proceed.insert(0, "..") |
There was a problem hiding this comment.
There are a couple of places where you create new lists that are essentially copies of existing ones:
- In the
-abranch:data_to_proceed = [f for f in files]just recreates the same list. - In the
-1branch: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
| if args.one: | ||
| output = [f for f in data_to_proceed] | ||
| for f in output: | ||
| print(f) |
There was a problem hiding this comment.
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
| 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)) |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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
| prog="wc", | ||
| description="Counts words in a file that contain a particular character", | ||
| ) |
There was a problem hiding this comment.
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
| # lines count | ||
| if args.l: | ||
| num_lines = len(lines) | ||
| output_for_one_file.append(num_lines) |
There was a problem hiding this comment.
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
| # 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) | ||
|
|
There was a problem hiding this comment.
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
| 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)) |
There was a problem hiding this comment.
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
|
Thanks for opening a new PR! AI started to review it |
| 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: |
There was a problem hiding this comment.
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?
| prog="wc", | ||
| description="Counts words in a file that contain a particular character", | ||
| ) |
There was a problem hiding this comment.
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?
| elif len(args.path) == 1: | ||
| num_cols = len(output[0]) - 1 | ||
| column_widths = [len(str([output[0][i]])) for i in range(num_cols)] |
There was a problem hiding this comment.
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?
| # lines count | ||
| if args.l: | ||
| num_lines = len(lines) | ||
| output_for_one_file.append(num_lines) |
There was a problem hiding this comment.
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.”
| # 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) | ||
|
|
There was a problem hiding this comment.
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.
| # Right-align with 1 space padding (like wc) | ||
| line_parts.append(str(row[i]).rjust(column_widths[i] + 6)) |
There was a problem hiding this comment.
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?
Learners, PR Template
Self checklist
Changelist
Implemented shell tools in Python
Questions
what formatter to use for python?