LONDON SDC | Anna Fedyna | Module-Tools | Week 4 | Implement Shell Tools in Python #51
Conversation
There was a problem hiding this comment.
Thank you for your PR! I'm Borja, a volunteer, and I'll be reviewing your code.
The implementations of cat, ls, and wc are good! I have left a few edge cases that are not covered, as well as a few suggestions, but you're definitely on the right track :)
Keep up the good work!
| parser.add_argument("-n", help="Number the output lines, starting at 1.", default = None, action="store_true") | ||
| parser.add_argument("-b", help="Number the non-blank output lines, starting at 1.", default = None, action="store_true") |
There was a problem hiding this comment.
What happens if neither -n nor -b are used?
For instance, what will your solution print if we invoke it like this: python3 cat.py sample-files/1.txt?
| except FileNotFoundError: | ||
| print(f"Error: File '{path}' not found.") |
There was a problem hiding this comment.
Very good catching of the error cases, this really improves the user's experience. Congrats!
| return len(content.splitlines()) | ||
|
|
||
| def count_words(content): | ||
| return len(re.findall("[a-zA-Z\-\.'/]+", content)) |
There was a problem hiding this comment.
Whenever I run this solution, I see the following warning:
implement-shell-tools/wc/wc.py:22: SyntaxWarning: invalid escape sequence '\-'
return len(re.findall("[a-zA-Z\-\.'/]+", content))
While the code seems to work even with the warning, could you elaborate on what could be the reason for the warning?
| except Exception as e: | ||
| print(f"An unexpected error occurred: {e}") | ||
|
|
||
| if len(args.paths) > 1: |
There was a problem hiding this comment.
When I pass more than one line and any flag, I get the following error: An unexpected error occurred: name 'no_options' is not defined.
For instance, with this invocation:
➜ python3 wc.py -l sample-files/*
1 sample-files/1.txt
An unexpected error occurred: name 'no_options' is not defined
1 sample-files/2.txt
An unexpected error occurred: name 'no_options' is not defined
5 sample-files/3.txt
An unexpected error occurred: name 'no_options' is not defined
Traceback (most recent call last):
File "/Users/blorente/code/github.com/CodeYourFuture/Module-Tools/implement-shell-tools/wc/wc.py", line 65, in <module>
if no_options:
^^^^^^^^^^
NameError: name 'no_options' is not defined
What could be the cause of this?
| total_lines, total_words, total_bytes = 0, 0, 0 | ||
| for path in args.paths: | ||
| try: | ||
| file_path_to_procccess = os.path.join(cwd, path) |
There was a problem hiding this comment.
| file_path_to_procccess = os.path.join(cwd, path) | |
| file_path_to_proccess = os.path.join(cwd, path) |
Typo: There was an extra c :D
This is not relevant for the code working, but in a real code review it's likely to get pointed out, so I do it here as well. If you decide to change it, note that you'll have to change every place where that variable is used.
| except FileNotFoundError: | ||
| print(f"Error: File '{file_path_to_procccess}' not found.") |
There was a problem hiding this comment.
What happens if I pass three files, but the second one is missing? How will that affect the output?
For a stretch goal: How would you make it so that all errors are displayed at the end, instead of intermingled throughout the output?
| description='list directory contents') | ||
|
|
||
| parser.add_argument("-1", "--one", help="Force output to be one entry per line.", default = None, action="store_true") | ||
| parser.add_argument("-a", help="Include directory entries whose names begin with a dot (.).", default = None, action="store_true") |
There was a problem hiding this comment.
When I run the ls utility with -a, the current directory (.) and parent directory (..) are printed:
$ ls -a sample-files
. .. .hidden.txt 1.txt 2.txt 3.txt dirHow could you change your solution to make sure they are printed?
| else: | ||
| list_docs(cwd) |
There was a problem hiding this comment.
Good catch on the behaviour of ls without arguments! Well done.
| for doc in doc_to_output: | ||
| print(doc) | ||
| else: | ||
| print(' '.join(doc_to_output)) |
There was a problem hiding this comment.
In ls, files are printed with a certain amount of padding, so that if we have many files, they appear to be in a table:
➜ /bin/ls
ls.js package-lock.json README.md
node_modules package.json sample-files
This is not part of the assignment brief but, as a streacth goal, would you like to consider how to pad the outputs like that?
blorente
added
👀 Review Requirements
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.