-
Notifications
You must be signed in to change notification settings - Fork 49
Add psh (shell rainbow) and pv (pipeviewer) #75
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?
Conversation
|
I would like to code review this. |
|
I would like to code review this |
|
High Level Checks:
Overall the project is great, my only suggestions are improving documentation, and adding some error handling for empty inputs for pv.c. |
|
High-Level Checks: [✅] Are there tests included? [ ] Does the code follow project formatting standards? [✅] Does the OS compile and run successfully? [✅] Is documentation provided? Code Checks: [ ] Were all edge cases considered? As for pv, I noticed in pv_tests.c that the data passed to the function is a relatively short string rather than a file. I think the project could benefit from more extensive test cases that simulate real-world file handling. That being said, I was impressed by the thorough error checking, particularly in sh.c, which demonstrates solid attention to detail. [✅] Is the code well-organized and maintainable? [✅] Does the code maintain performance? Additional Comments: |
|
This is quite fun but also well-implemented. I had a great time messing around with psh. Thanks for doing this right, but also having some fun with it. |
Modifications:
/tmpto store files necessary forpshpsh.cpshvia file/tmp/psh_togglepv.ccat README.md | pv > new-README.mdpshcdbehavior to properly handle certain cases where final char is cut offLimitations:
pvdoes not show a progress barpshrequires 256-color support to work properly (this is supported by essentially every modern terminal, so it is functionally a non-issue)(This work was done by Andrew Moore (me) and Athene Marston (aemarston))