Skip to content

Conversation

@andywu42
Copy link

@andywu42 andywu42 commented Sep 24, 2024

This pull request enhances the rm command by adding flags to help the user quickly delete files and directories as they want. The flags added are:
-r recursively delete all files/directories in a directory
-i interactive mode where the user is prompted whether they want to delete each file
-f force, which does not ask and just deletes the file
-d deletes an empty directory
-v verbose

File(s) changed:
user/rm.c

Run 'rm' on its own to see how to use.

@rick2244
Copy link

I'm going to code review this

@riyamathur1
Copy link

I can code review this

@rick2244
Copy link

Checks on Rm command
works for -i and handles tests
works for -f; maybe include an error message if the file that the person is trying to delete forcefully doesn’t exist
works for -d, even has error checking for a directory that doesn’t exist
works for -r, and has good error checking
works for -v, and has good error checking

Code review:
Should have used fprintf for error checking on line 98, doesn’t print out intended message

seems like the error message on line 172 is being overwritten

Great Job!

High Level Checks:

  • [✔️] PR fully resolves the task/issue
  • Does the PR have tests? Could have probably created test files and directories for me to work with
  • [✔️] Does the PR follow project code formatting standards?
  • [✔️] Does the OS still compile and run?
  • [✔️] Is documentation included?

Code Checks:

  • [✔️] Does the code achieve its intended purpose?
  • [✔️] Were all edge cases considered?
  • [✔️] Is the code organized and maintainable?
  • [✔️] Does the code maintain the same level of performance? (no performance regressions)

@jlazaro2
Copy link

jlazaro2 commented Oct 3, 2024

i can code review this as well!

@jlazaro2
Copy link

jlazaro2 commented Oct 4, 2024

This project was well done and follows the objective, plus the added suggestions of the original issue. The code is well formatted and it was a nice use of recursion overall. The added flags also follow their purpose and enhance the rm command as needed. Here's a couple suggestions:

  • Add an error message whenever there is no argument following the flag. Instead, it tries to delete the flag itself and then produces an error message. Maybe print something like "No specific file given" when "rm -f" runs. S
  • Similar, to when a specific file does not exist. I would add a more detailed error message instead of "failed to delete"

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
    No tests files were made, but I was confused on how to run certain flags. Maybe just more
    specific instructions.
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
    Consider suggestions above
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@riyamathur1
Copy link

Code Review: Added flags to rm command

The enhancements to the rm command are well-implemented, and the flags work as expected. I found the code style clean and maintainable, with useful comments.

Flags:
-r (Recursive Deletion): Tested and works as expected. Able to recursively delete directories and their contents.
-i (Interactive Mode): Works correctly, I was prompted for each file before deletion.
-f (Force Deletion): Files are deleted without prompts, as intended.
-d (Empty Directory Deletion): Correctly deletes empty directories. Good error handling for the deletion of non-empty directories.
-v (Verbose Mode): Verbose output is displayed correctly for each deleted file/directory.

Code Style:
The code is easy to understand with javadoc comments. Consistent formatting is maintained throughout the code.

Edge cases:
The code correctly handles conflicting flags with error messages.

Suggestion:
A test file or some instructions for how to test the implementation (which directories/files to create, how to run flags) would be helpful.

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?
  • [✔️] Is documentation included?

Code Checks:

  • [✔️] Does the code achieve its intended purpose?
  • [✔️] Were all edge cases considered?
  • [✔️] Is the code organized and maintainable?
  • [✔️] Does the code maintain the same level of performance? (no performance regressions)

@jcchu0
Copy link

jcchu0 commented Oct 6, 2024

Will review this.

Similar to own project, flags.

@jcchu0
Copy link

jcchu0 commented Oct 6, 2024

Nicely done project.
Code is well written and easy to follow, nicely formatted and documented as well.

Suggestion:

  • Allowing for handling multiple flags in a single command line argument (if applicable/viable, but can see use for it in some cases)
  • Add additional error handling messages for specific error.
  • Dedicated test file(s) would be nice, and documentation of how to test it according to the changes and additions you made.
  • -i flag options on display (y/n) etc., choices available to user when in interactive mode

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?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

@malensek
Copy link
Contributor

Looks like the most recent changes broke the build, since you can't check in an empty directory I had to mkdir empty before I could build.

This looks great. There are a few small issues I found:

  • The flags as variables probably make sense to you, but it was really hard to figure out what some of them meant. Would be better as a struct of options or with more descriptive names.
  • Why read 32 characters when prompting for y/n?
  • Check whether the file exists or not before prompting in interactive mode. If it's not there, no need to prompt.
  • Would be nice to tell the user the directory wasn't empty when they try using -d. Right now it correctly reports that the operation failed, but usually rm explains why.
  • Looks like you fixed a few of the small issues mentioned already! Great.

These are all really small problems. This is quality work!

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.

6 participants