Glasgow | 25-SDC-July | Sheetal Kharab | Sprint 4 | implement-shell tools in python #153
Conversation
sheetalkharab
added
the
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work so far on this. I've left some comments to help
| # set argument parser | ||
| parser = argparse.ArgumentParser(description="Python implementation of cat command") | ||
|
|
||
| # to number all lines |
There was a problem hiding this comment.
If these comments were not here, would it be obvious what these lines of code were doing?
There was a problem hiding this comment.
Thank you, Yes these lines are self explainatory and comment are not required here.
There was a problem hiding this comment.
What is the purpose of the -1 argument here? Can you see any issues regarding your implementation with -1?
There was a problem hiding this comment.
The argument is defined but unused; currently, the program always prints one file per line, so -1 doesn’t change anything.
There was a problem hiding this comment.
Hello @LonMcGregor, sorry for late response. when i am checking these file I found about these comments. so I made some changes acc to your suggestions. Sorry once again.
There was a problem hiding this comment.
There are a couple of issues here:
- Think about all the combinations of arguments a user might use - do you spot any issues?
- In your code there seems to be some duplication. Can you find where it is and develop a solution?
There was a problem hiding this comment.
yes, you are right. The code only handles one flag at a time because it uses if…elif…else, so combinations like -l -w don’t work. Also, the printing logic is duplicated for per-file and total counts. I just made some chnae sacc to your suggestions. Thank you.
LonMcGregor
added
Reviewed
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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). |
4 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. 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). |
sheetalkharab
added
the
Needs Review
LonMcGregor
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
code for ls, wc, cat
Questions
no