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 26 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?
| 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?
| 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?
| 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?
| 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?
| 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?
LonMcGregor
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1152
Changelist
Implemented wc, ls, cat in python