-
Notifications
You must be signed in to change notification settings - Fork 25
Project 1: Enhanced cat with -n/-b/-E/-s options and documentation #29
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 will review this PR. |
pjpatil30
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.
Looks great! Functionality works as expected in most cases. Documentation was present, but in the README.md, instead of a new file in docs called cat.md. So first recommendation would be to move the documentation there!
I created a new test file as well with more blank lines in between to double check functionality worked. All the individual additions (-n, -b, -E, -s) all work as expected. However, with the multiple one for cat -nEs, it adds an extra number at the end of the file. In my test file, there is a blank line at the end, but -nEs should get rid of all the blank lines and number the remaining lines (as it does through the rest of the file). So, it successfully does it for every line before the last line, just a minor issue with the last line.
Another edge case I caught is when you do -bEs, if the file has blank spaces in between the lines, there is some error with the numbering process (ie it skips over some of the lines).
Other than that, everything else looks good! It is clean, reusable code, and has good comments throughout making it easy to follow.
High Level Checks:
- PR fully resolves the task/issue
- Does the PR have tests?
- Does the PR follow project code formatting standards?
- Does the OS still compile and run?
- [Partially] Is documentation included? <-- Just fix the location.
Code Checks:
- Does the code achieve its intended purpose?
- [Mostly] Were all edge cases considered? <-- Just the two mentioned above.
- Is the code organized and maintainable?
- Does the code maintain the same level of performance? (no performance regressions)
|
The code looks great! I can see how the code is progressing and what's happening with it. The documentation, including user/test.txt, Readme.md. I also recommend creating a cat.md file by itself and moving it under the docs folder. Second, I suggest adding one more test file to check multiple empty lines. I created file.txt Line 4: After empty line Line 6: Multiple consecutive empty lines Line 10: End of file" When i test for -s flag, cat -s file.txt, it printed which I believed should be Line 4: After empty line Line 6: Multiple consecutive empty lines Line 10: End of file" High Level Checks:
Code Checks:
|
|
Verification Testing - -b flag for blank lines: I tested cat -b test.txt on a file with some empty lines, but the line numbering seemed off. It seems like either it runs with lines that should be skipped or the count gets messed up somewhere. Looking at line 18 in the code, the logic checks to see if a line is empty or not, but this only looks at the first character. If a line starts with a newline, it might not handle it correctly. - Combining flags: When I tried cat -nb test.txt, the output was confusing because both the -n and -b flags were active at the same time. I see tht the code lets you set both flags , but they conflict with each other since, and i think that the program should probably prevent or warn the user from using both conditions at once. - Test file: The test file is pretty consistent, but it might be better to include more cases such as blank lines, mixed content, or different edge cases to properly test the flags. Code Walkthrough Formatting High Level Checks: |
|
Great work. The CRs here do a great job giving you a roadmap for what to fix, this is one of the few that I can't say there's much to add beyond what was already covered above! 4.5/5 +0.5 -> address the issues raised by the reviewers above. |
Description
This PR implements Project 1 for CS 326 by enhancing the cat user-space command in FogOS/xv6.
New features
-n – number all output lines
-b – number nonempty output lines only
-E – display $ at the end of each line
-s – squeeze multiple consecutive blank lines into a single blank line
Testing
Commands verified inside QEMU:
cat -n test.txt # number all lines
cat -b test.txt # number nonempty lines only
cat -E test.txt # show $ at line ends
cat -s test.txt # squeeze consecutive blank lines
cat -nEs test.txt # combine options
echo "abc" | cat -n # read from stdin
cat -n file1 file2 # multiple files
Edge cases tested: empty file, consecutive blank lines, multiple files, stdin input.
make clean && make qemu builds and runs successfully with no regressions.
Checklist
✔Documentation included (README.md with build/run/test instructions)
✔PR fully resolves the Project 1 proposal
✔Manual tests included and verified inside QEMU
✔Code follows xv6/FogOS formatting standards
✔OS compiles and runs without errors