Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions implement-shell-tools/cat/cat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import glob
import argparse

def cat(filepath, n=False, b=False, line_counter=None):
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

try:
with open(filepath) as f:
for line in f:
if b:
if line.strip():
print(f"{line_counter[0]:6}\t{line}", end='')
line_counter[0] += 1
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

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

def main():
Copy link
Member

Choose a reason for hiding this comment

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

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

parser = argparse.ArgumentParser(description = "Concatenate files and print on the standard output.")
parser.add_argument('-n', action='store_true', help='number all output lines')
parser.add_argument('-b', action='store_true', help='number non-empty output lines')
parser.add_argument('files', nargs='+', help='files to concatenate')
args = parser.parse_args()

files = []
for pattern in args.files:
files.extend(glob.glob(pattern) or [pattern])
Copy link
Member

Choose a reason for hiding this comment

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

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?


line_counter = [1]
for file in sorted(files):
cat(file, args.n, args.b, line_counter)

if __name__ == "__main__":
main()
28 changes: 28 additions & 0 deletions implement-shell-tools/ls/ls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import os
import argparse

def ls(path='.', one_column=False, show_hidden=False):
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above about default args

try:
files = os.listdir(path)
if not show_hidden:
files = [f for f in files if not f.startswith('.')]
files.sort()

if one_column:
print(*files, sep='\n')
else:
print(*files)
except FileNotFoundError:
print(f"ls: cannot access '{path}': No such file or directory")

def main():
parser = argparse.ArgumentParser()
parser.add_argument('-1', dest='one_column', action='store_true', help='list one file per line')
parser.add_argument('-a', action='store_true', help='show hidden files')
parser.add_argument('path', nargs='?', default='.', help='directory to list')
args = parser.parse_args()

ls(args.path, args.one_column, args.a)

if __name__ == "__main__":
main()
37 changes: 37 additions & 0 deletions implement-shell-tools/wc/wc.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import argparse

def wc(path, count_lines, count_words, count_bytes):
try:
with open(path, 'r') as f:
content = f.read()
lines = content.splitlines()
words = content.split()
bytes_ = len(content.encode('utf-8'))

parts = []
if count_lines: parts.append(str(len(lines)))
if count_words: parts.append(str(len(words)))
if count_bytes: parts.append(str(bytes_))

if not parts:
Copy link
Member

Choose a reason for hiding this comment

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

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)

parts = [str(len(lines)), str(len(words)), str(bytes_)]
print(' '.join(parts), path)

except FileNotFoundError:
print(f"wc: {path}: No such file or directory")
except IsADirectoryError:
print(f"wc: {path}: Is a directory")

def main():
parser = argparse.ArgumentParser()
parser.add_argument('-l', action='store_true', help='Count lines')
parser.add_argument('-w', action='store_true', help='Count words')
parser.add_argument('-c', action='store_true', help='Count bytes')
parser.add_argument('paths', nargs='+', help='Files to count')
args = parser.parse_args()

for path in args.paths:
Copy link
Member

Choose a reason for hiding this comment

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

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

wc(path, args.l, args.w, args.c)

if __name__ == "__main__":
main()