London | 26-SDC-July | Alex Jamshidi | Sprint 4 | Implement shell tools in python - #676
London | 26-SDC-July | Alex Jamshidi | Sprint 4 | Implement shell tools in python#676Alex-Jamshidi wants to merge 30 commits into
Conversation
…d through functions as arguments - also fixed an if boolean
Alex-Jamshidi
added
📅 Sprint 4
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this task. Reading through it, I can see you have put thought into these implementations, but they feel a little bit over-engineered, and at times are a bit difficult to see how the data is flowing through them. I've got a few questions for you to answer which might help me better understand your thinking.
|
|
||
| # ===== Flag Handling ===== | ||
| def execute_flags(flag_status): | ||
| if getattr(args, "1"): |
There was a problem hiding this comment.
Here, when you are doing things like if args.a …, then you set something to True. What benefit is there doing it this way over just referencing args.a when you need it?
There was a problem hiding this comment.
On reflection - I've been tempted to accept this change as it would remove a decent amount of redundant code. However the one advantage I find from it is readability. It can be easy to forget what the flags do, and the current code creates clearly named booleans.
| flag_status = {"print_in_list": False, "show_all": False} | ||
| execute_flags(flag_status) | ||
|
|
||
| cwd = os.getcwd() |
There was a problem hiding this comment.
What do you use the cwd for in this program?
There was a problem hiding this comment.
Ah! I wasn't familiar enough with the Path library. I see that it already gives a relative path to the cwd, so giving it the cwd is redundant. I've updated the code to reflect this which tidies things up quite a bit, and used Path.cwd() where needed
| if not (Path(cwd) / p).is_dir() | ||
| ] | ||
| if not flag_status["show_all"]: | ||
| file_args = remove_dot_files(file_args) |
There was a problem hiding this comment.
There are multiple places where you use remove_dot_files in this program. Is there a reason for that?
There was a problem hiding this comment.
The program takes the original filesystem items (which could be files or folders, or both).
It then splits these items into files (get_file_args) and directories (get_dir_args).
The list of file arguments may contain dot files, like .hidden.txt, so these are removed at this stage.
If directory items are found, these need to be opened up, the contents of which are to be displayed on a separate line, this is done using the dir_output function. These directories may also contain dot files, so these need to be removed.
If the second instance of remove_dot_files were removed:
In the case of the sample files given, none of the sub directories actually contain dot files, so the output of ls and ls.py would be equivalent.
but if I ran, for example: python3 ls.py ./* whereby sample-files is now a subdirectory to be opened.
then when the dir_output function populates the files in sample-files, the .hidden.txt would be incorrectly shown.
| return all_files_data | ||
|
|
||
| def read_file(file_name, cwd): | ||
| file_path = Path(cwd) / file_name |
There was a problem hiding this comment.
Why are you using the Path / expression here?
There was a problem hiding this comment.
I have changed this code throughout the program to use Path more efficiently, equivalent to my changes in ls.py. Here I now pass Path(file_name).
I also replaced os.path.getsize(file_path) with Path(file_path).stat().st_size
Removing the need for the os library at all.
| return len(text.splitlines()) | ||
|
|
||
| def calculate_word_count(text): | ||
| return len(text.split()) |
There was a problem hiding this comment.
Is there a reason the word count method has a different structure to your line count method?
There was a problem hiding this comment.
I realise
if not text:
return 0
is redundant and have removed it.
| for file in output_data: | ||
| output_string = "" | ||
|
|
||
| for metric in metrics: |
There was a problem hiding this comment.
Do you need to loop over metrics and then check if it is in displayed_metrics? Could you simplify this?
There was a problem hiding this comment.
I struggled a bit with this, I have updated it to a slightly better way to do things.
LonMcGregor
added
Reviewed
Alex-Jamshidi
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1152
Changelist
Implemented wc, ls, cat in python