-
-
Notifications
You must be signed in to change notification settings - Fork 42
Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 4 | Implement shell tools (cat, ls, wc) in Python #285
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?
Sheffield | 25-SDC-Nov | Hassan Osman | Sprint 4 | Implement shell tools (cat, ls, wc) in Python #285
Conversation
… is blak and if so removes it
…for -n & -b options
…when -a option not added by end user
…ent to diplay in one line with spaces in between
…ual ls with -1 flag
…ts and updating total
OracPrime
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.
You've made it very hard to review - that in itself is a problem. Don't commit individual file changes, commit self-consistent changes to your code. I should be able to pull any commit and it should work. I should be able to look at a single commit and see what the big change is.
At least one comment needs addressing - the double space in the ending.
implement-shell-tools/cat/cat.py
Outdated
|
|
||
| for line in lines: | ||
| if args.n: | ||
| print(f"{line_number} {line}") |
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.
If the function of software is something else, then the formatting of the output may be less important. But the purpose of this software is to present information to the user, so it needed sorting
implement-shell-tools/ls/ls.py
Outdated
| print(item) | ||
| else: | ||
| print(item, end=" ") | ||
| print(item, end=" ") |
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.
Why two spaces? Should it be \t? I don't actually know, but I think you should find out :)
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.
replaced the double spacing with /t. Tested it and worked. Thank you!
implement-shell-tools/wc/wc.py
Outdated
|
|
||
|
|
||
| def process_file(file_path): | ||
| global total_lines, total_words, total_characters, file_count |
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.
global is rarely a good word to see in source code... what's it doing 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.
the goal of having it so that total_lines, total_words, total_characters, file_count can be used inside my helper function otherwise, an error would've been thrown for them not being defined locally. Anyway, I am looking for alternatives in the meantime.
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.
A function can return more than one value. You can define them in the function and them have them all on the return statement. For example
def fun():
return "stuff", 20
s, x = fun()
print(s)
print(x)
implement-shell-tools/wc/wc.py
Outdated
|
|
||
| if args.line: | ||
| print(f"{stats['lines']} {path}") | ||
| print(f"{stats['lines']} {file_path}}") |
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.
Unless I'm seeing some distorted github view of reality, it looks like you are committing individual file changes as one commit each. You don't want to do that. Each commit should be a group of file changes which is logically consistent.
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.
My bad, I thought it frequent committing is the way. I will aim to group my commits moving forward. Thank you!
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.
Frequent commits are good, but every commit should be a logical change: the commit description should describe the group of changes being made, but it's all the changes need for a particular step forward, not file by file
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.
Thank you!
implement-shell-tools/ls/ls.py
Outdated
|
|
||
| parser.add_argument("-1", "--one", dest="one", help="Output one entry per line", action="store_true") | ||
| parser.add_argument("-a", help="List all files & directories, including hidden ones", action="store_true") | ||
| parser.add_argument("paths", nargs="*", default=["."], help="The file path to read from") |
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.
Arg is paths, comment is path. That will confuse people.
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.
Reworded the confusing bit. Hope it's good now.
…ct ls do when listing files in a dir
…own and get rid of code no longer in use
OracPrime
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.
Nice work. I think you're done with this one now!
Self checklist
This PR provides implementations of shell tools: cat, ls, and wc in Python.