London | SDC-Nov-2025 | Ikenna Agulobi | Sprint 4 | Implement shell tools python#301
London | SDC-Nov-2025 | Ikenna Agulobi | Sprint 4 | Implement shell tools python#301ike-agu wants to merge 6 commits intoCodeYourFuture:mainfrom
Conversation
ike-agu
added
the
Needs Review
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-4) doesn't match expected format (example: 'Sprint 2', without quotes) If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
github-actions
bot
removed
the
Needs Review
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-4) doesn't match expected format (example: 'Sprint 2', without quotes) If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
ike-agu
changed the title
ike-agu
added
the
Needs Review
DaryaShirokova
added
Review in progress
DaryaShirokova
left a comment
DaryaShirokova
left a comment
There was a problem hiding this comment.
Looks good! Just added a few comments
| python cat.py sample-files/*.txt | ||
|
|
||
| * Number every line across multiple files(including empty ones) | ||
| python cat.py -n sample-files/*.txt |
There was a problem hiding this comment.
There is a small discrepancy in how you program handles some cases it seems:
$ cat -n sample-files/*.txt
1 Once upon a time...
2 There was a house made of gingerbread.
3 It looked delicious.
4 I was tempted to take a bite of it.
5 But this seemed like a bad idea...
6
7 There's more to come, though...
$ python cat.py -n sample-files/*.txt
1 Once upon a time...
2
3 There was a house made of gingerbread.
4
5 It looked delicious.
6 I was tempted to take a bite of it.
7 But this seemed like a bad idea...
8
9 There's more to come, though...
10
There was a problem hiding this comment.
Thanks for pointing out the discrepancy. I’ve fixed the line-numbering logic so empty lines are handled correctly, matching cat behaviour. Thank you.
implement-shell-tools/cat/cat.py
Outdated
| @@ -0,0 +1,50 @@ | |||
| import argparse | |||
| # ------------------------------------------- | |||
There was a problem hiding this comment.
It's ok to have comments, but it would be even better to have the code split into methods with descriptive names :)
There was a problem hiding this comment.
Thanks for your review and suggestions. I’ve refactored my code by splitting it into small functions with descriptive names.
implement-shell-tools/cat/cat.py
Outdated
| content = f.read() | ||
|
|
||
| lines = content.split("\n") |
There was a problem hiding this comment.
Maybe check if you could use readlines method here.
| return args.paths | ||
|
|
||
| # list a single directory | ||
| def list_directory(directory_path, show_hidden, file_per_line): |
There was a problem hiding this comment.
It is good as is, but as a suggestion - a tiny bit cleaner approach would be to first get the list of directories/files and for list_directory function to just accept this list and print it (filtering whatever is not needed). This would follow 1 method/1 responsibility paradigm (which helps with code readability / reusability).
|
|
||
|
|
||
| def main(): | ||
| # -------------------------------------------------------- |
There was a problem hiding this comment.
Again, maybe try splitting into functions instead of comment blocks instead :)
| # -------------------------------------------------------- | ||
| # Print totals if there are multiple files | ||
| # -------------------------------------------------------- | ||
| if len(args.paths) > 1: |
There was a problem hiding this comment.
Maybe there is a way to reuse the same functions (with different args) for printing individual lines and totals
| python wc.py sample-files/* | ||
|
|
||
| * Just lines | ||
| python wc.py -l sample-files/3.txt |
There was a problem hiding this comment.
Looks like some outputs are different?
wc sample-files/*
1 4 20 sample-files/1.txt
1 7 39 sample-files/2.txt
5 24 125 sample-files/3.txt
7 35 184 total
$ python wc.py sample-files/*
2 4 20 sample-files/1.txt
2 7 39 sample-files/2.txt
6 24 125 sample-files/3.txt
10 35 184 total
2 participants
Learners, PR Template
Self checklist
Changelist
This PR contains my implementation of cat, ls and wc shell tool commands in python