London | 26-SDC-Mar | Khilola Rustamova| Sprint 3 |Implement shell tools python#537
London | 26-SDC-Mar | Khilola Rustamova| Sprint 3 |Implement shell tools python#537HilolaRustam wants to merge 5 commits into
Conversation
HilolaRustam
added
Needs Review
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking good, and like things work :) I left a few style comments, and many of my comments to one project apply to the others as well.
| return sorted(files) | ||
|
|
||
| def read_lines(file): | ||
| with open(file, "r", encoding="utf-8") as f: |
There was a problem hiding this comment.
This block is more indented than the others - why is that?
| try: | ||
| lines = read_lines(file) | ||
| except FileNotFoundError: | ||
| print(f"cat: {file}: No such file or directory", file=sys.stderr) |
There was a problem hiding this comment.
Good job printing to stderr not stdout :)
However! If you ran into an issue here, I think your program will still exit with exit code 0. What exit code do you think it should exit with?
| sys.stdout.write(prefix + line) | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
This all works, but often people will have their main function at the top of the file so it's the first thing you read, and allow you to scroll down to read details if needed. You don't need to follow this, but it can often make it easier to read/review code.
| paths.append(a) | ||
|
|
||
| files = expand_paths(paths) | ||
| print_lines(files, number_all,number_nonempty) |
There was a problem hiding this comment.
I've noticed your whitespace is a bit inconsistent (e.g. no space after this ,) - you may want investigate a tool like black to help automate fixing this.
| number_all = True | ||
| elif a == "-b": | ||
| number_nonempty = True | ||
| number_all = False #-b overrides -n |
There was a problem hiding this comment.
What would happen if -b appeared before -n in the command line?
| paths.append(a) | ||
|
|
||
| files = expand_paths(paths) | ||
| print_lines(files, number_all,number_nonempty) |
There was a problem hiding this comment.
We often used named arguments in Python when we have multiple arguments of the same type, to make sure we don't accidentally pass them in the wrong order. If you wrote print_lines(files, number_nonempty, number_all) here it wouldn't be obvious that the order was swapped. Instead we can write:
print_lines(files, number_all=number_all, number_nonempty=number_nonempty)and if we swapped them by mistake it would be obvious.
| def list_dir(path, show_all=False, one_per_line = False): | ||
| try: | ||
| entries = os.listdir(path) | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
Same question as above about exit codes.
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
implementation of cat, ls and wc with python
Questions