Skip to content

Conversation

@abkslm
Copy link

@abkslm abkslm commented Sep 18, 2024

Modifications:

  • init.c:
    • creates a directory /tmpto store files necessary for psh
  • created psh.c
    • toggles psh via file /tmp/psh_toggle
  • created pv.c
    • allows data to be piped in and displays total time to pass through pipe
    • example usage: cat README.md | pv > new-README.md
  • sh.c:
    • added ANSI codes, logic and functions for psh
    • modified cd behavior to properly handle certain cases where final char is cut off

Limitations:

  • pv does not show a progress bar
  • psh requires 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))

@mikesmith-23
Copy link

I would like to code review this.

@fhshaik
Copy link

fhshaik commented Sep 27, 2024

I would like to code review this

@fhshaik
Copy link

fhshaik commented Oct 1, 2024

High Level Checks:

  • PR fully resolves the task/issue
    Both psh and pv work wonderfully, and I think that psh especially adds so much flair to the shell.
  • Does the PR have tests?
    While there are no test cases within the OS for pv, it is simple to use with an example test given. Psh does not need any test cases and works well.
  • Does the PR follow project code formatting standards?
    The formatting is fine, obeying the relevant code formatting standards and being consistent with other code in the files modified.
  • Does the OS still compile and run?
    Yes it does.
  • Is documentation included?
    Documentation is included for the main files added, but for the modified files such as sh.c, the commenting needs to be more robust as there are added functions left uncommented. These functions are small, but I think some added documentation would help.
    Code Checks:
  • Does the code achieve its intended purpose?
    It does achieve its intended purpose.
  • Were all edge cases considered?
    psh.c does not take input, but it seems to handle error cases sufficiently. pv.c does not work well when you pass no input into it. The code takes you into an infinite loop where hitting enter only increases the number of "bytes transferred" that prints onto the screen. I had to exit the terminal through Ctrl+A X.
  • Is the code organized and maintainable?
    The code is sufficiently organized, but I think there needs to be more clear delineation of psh code inside sh.c, so we know what to modify if we want to add onto the utility. Otherwise, all code is organized, maintainable, and not difficult to read.
  • Does the code maintain the same level of performance? (no performance regressions)
    psh does not add any noticeable performance changed. pv also does not add any noticeable changes.

Overall the project is great, my only suggestions are improving documentation, and adding some error handling for empty inputs for pv.c.

@mikesmith-23
Copy link

mikesmith-23 commented Oct 3, 2024

High-Level Checks:
[✅] Does the PR fully resolve the task/issue?
Yes, both the psh and pv commands are fully functional and meet the specifications laid out in the initial project proposal. The implementation aligns well with the intended goals.

[✅] Are there tests included?
Yes, there are well-defined tests for each command, with a comprehensive built-in test suite for the pv command. The test cases are clear and demonstrate effective coverage for expected behavior.

[ ] Does the code follow project formatting standards?
For the most part, the code is well-structured and easy to read. However, sh.c could benefit from another pass. There are a few extraneous whitespaces that can be removed, and some of the functions are missing Javadoc-style comments for better documentation.

[✅] Does the OS compile and run successfully?
Yes, the OS boots and runs as expected when executing make qemu. Everything compiled and operated without issues during testing.

[✅] Is documentation provided?
While the PR includes a solid overview of the work, it would be helpful to have a more detailed README.md with additional project context and usage instructions. This would improve the ease of use and understanding for future contributors.

Code Checks:
[✅] Does the code accomplish its intended purpose?
Yes, both psh and pv are working as intended. They provide useful and user-friendly functionality within the OS environment.

[ ] Were all edge cases considered?
One issue I noticed is that the color theme used in psh persists into gojira after the OS is closed. It might be worth investigating if there's a way to reset this before the user exits the OS. On the flip side, I do like how the color toggle remains consistent when re-entering the OS, as it adds a nice personalized touch.

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?
The code is generally easy to follow and well-structured. I was able to navigate through the files without difficulty, and the logic is laid out clearly.

[✅] Does the code maintain performance?
During my review, I didn’t encounter any noticeable performance issues. Everything ran smoothly, with no signs of lag or glitches.

Additional Comments:
I particularly enjoyed using the psh command. It adds a fun, colorful dimension to the OS. As a suggestion, it might be interesting to expand on this by allowing the user to toggle color variations line-by-line or even character-by-character. This could make the terminal even more stylized and dynamic. Overall, fantastic work—very cool! 🌈

@malensek
Copy link
Contributor

malensek commented Oct 24, 2024

This is quite fun but also well-implemented. I had a great time messing around with psh. pv is well done, and although I am sad I didn't get to see a magical progress bar (as I pleaded for in class ;-P), the current output is good already.

Thanks for doing this right, but also having some fun with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants