ZA | 25-SDC-July | Luke Manyamazi | Sprint 4 | Python Implement Shell Tools Exercises #148
Conversation
Luke-Manyamazi
added
the
Needs Review
illicitonion
added
Review in progress
| import glob | ||
| import argparse | ||
|
|
||
| def cat(filepath, n=False, b=False, line_counter=None): |
There was a problem hiding this comment.
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?
| else: | ||
| print(line, end='') | ||
| elif n: | ||
| print(f"{line_counter[0]:6}\t{line}", end='') |
There was a problem hiding this comment.
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?
|
|
||
| files = [] | ||
| for pattern in args.files: | ||
| files.extend(glob.glob(pattern) or [pattern]) |
There was a problem hiding this comment.
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?
| if b: | ||
| if line.strip(): | ||
| print(f"{line_counter[0]:6}\t{line}", end='') | ||
| line_counter[0] += 1 |
There was a problem hiding this comment.
Why is this value in an array? You always seem to look at exactly one value in the array?
| import glob | ||
| import argparse | ||
|
|
||
| def cat(filepath, n=False, b=False, line_counter=None): |
There was a problem hiding this comment.
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.
| except FileNotFoundError: | ||
| print(f"cat: {filepath}: No such file or directory") | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
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
| import os | ||
| import argparse | ||
|
|
||
| def ls(path='.', one_column=False, show_hidden=False): |
There was a problem hiding this comment.
Same question as above about default args
| if count_words: parts.append(str(len(words))) | ||
| if count_bytes: parts.append(str(bytes_)) | ||
|
|
||
| if not parts: |
There was a problem hiding this comment.
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)
| parser.add_argument('paths', nargs='+', help='Files to count') | ||
| args = parser.parse_args() | ||
|
|
||
| for path in args.paths: |
There was a problem hiding this comment.
If I pass multiple files, the real wc outlines a line that you don't - can you add that too?
illicitonion
added
Reviewed
Luke-Manyamazi
force-pushed
the
python-shell-tools-exercises
branch
from
35a4349 to
ff86f10
Compare
Learners, PR Template
Self checklist
Changelist
This PR implements basic versions of the following Unix commands in Python:
These scripts were designed to mimic standard command-line tools and handle typical edge cases like missing files and glob patterns.
Questions