London | Nadika Zavodovska | Module-Tools | Sprint 4 | Implement Shell Tools in Python #59
Conversation
nadika-zavodovska
added
Needs Review
OracPrime
added
Review in progress
implement-shell-tools/cat/cat.py
Outdated
| # loopp through each line | ||
| for line in file_object: | ||
| # rstrip("\n") - removes any trailing newline character (\n) from the end of each line | ||
| line_to_print = line.rstrip("\n") |
There was a problem hiding this comment.
This approach works, but you might want to consider how you could make things more efficient by using the end parameter on your print statements and not need to strip the line
| # For converting the given path to an absolute path | ||
| from pathlib import Path | ||
| # For colouring terminal output (blue for directories) | ||
| from colorama import Fore, Style, init |
There was a problem hiding this comment.
Colorama isn't a standard library is it? Doesn't this mean you need a requirements.txt?
implement-shell-tools/ls/ls.py
Outdated
| # Setup argument parser. Creates an instance(object) from built-in ArgumentParser class | ||
| parser = argparse.ArgumentParser(prog="ls command", | ||
| description="Implement 'ls' command with -1 and -a flags", | ||
| epilog="Now you can see the files in chossen path" |
There was a problem hiding this comment.
I'm guessing you don't have spell-check turned on in your editor?
| from colorama import Fore, Style, init | ||
|
|
||
| # Sets up colorama for blue colour for directories | ||
| init() |
There was a problem hiding this comment.
It's generally considered best practice to have function definitions first, top level code at the end of the file. It makes it easier if all of the top level logic is in one place, and where another coder would expect to find it.
|
|
||
| # Get absolute path | ||
| absolute_path = Path(args.path).resolve() | ||
| list_directory_files(str(absolute_path), display_hidden_files=args.a, display_one_per_line=args.one_per_line) |
There was a problem hiding this comment.
You convert the Path object to a string and pass it to your function. An alternative you might want to consider (I'm not saying you have to do it for this exercise, but it might be a good learning experience) having your function take a Path argument instead of a string, and use the Path functions to do a lot of the work inside the function
implement-shell-tools/wc/wc.py
Outdated
| import os | ||
|
|
||
| # Setup argument parser. Creates an instance(object) from built-in ArgumentParser class | ||
| parser = argparse.ArgumentParser( |
There was a problem hiding this comment.
As in previous file, you really ought to have top level code after function definitions
OracPrime
added
Reviewed
@OracPrime David, thank you for reviewing my code and for the helpful feedback! |
|
Those new commits all look good, well done. |
Thank you, @OracPrime David! |
Hello,
I've done all the tasks for the Implement-Shell-Tools folder.
Self checklist
Thank you.