-
-
Notifications
You must be signed in to change notification settings - Fork 42
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:main from
Conversation
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.
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.
@DaryaShirokova
DaryaShirokova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just added a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check if you could use readlines method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, maybe try splitting into functions instead of comment blocks instead :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a way to reuse the same functions (with different args) for printing individual lines and totals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Learners, PR Template
Self checklist
Changelist
This PR contains my implementation of cat, ls and wc shell tool commands in python