-
-
Couldn't load subscription status.
- Fork 27
London | 25-SDC-July | Eyuel Abraham | Sprint 4 | Implement shell tool python #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
London | 25-SDC-July | Eyuel Abraham | Sprint 4 | Implement shell tool python #154
Conversation
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.
Good work on these tasks so far, I've left some pointers to get you started on improvements
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 looks like the formatting of the output might not be working quite right. Can you find out why and fix it?
(Hint: Try running with a wildcard for all the files in the sample directory)
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.
This is a very good implementation, but I think you might have misread -1 in the readme as -l. You can keep -l, which is a good stretch task, but can you add the implementation for -1 as well?
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.
This implementation works well but you might be able to improve your code:
- Is there a reason you create variables that store data that is already stored in the args dictionary?
- Can you see where there is duplication in your code. Can you fix that?
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.
Creating show_line, show_word, and show_char isn’t necessary, since you can reference args.line, args.word, and args.character directly. The show_all value can also be derived from these flags on the fly. Thank you for flagging it.
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.