Skip to content
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

Add IO functions for removing files and directories #418

Open
Tracked by #416
kings177 opened this issue Aug 20, 2024 · 2 comments
Open
Tracked by #416

Add IO functions for removing files and directories #418

kings177 opened this issue Aug 20, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@kings177
Copy link
Member

kings177 commented Aug 20, 2024

Implement a file deletion functionality to give Bend the ability to delete files from the filesystem. This should include both single file deletion and recursive directory deletion.

1. delete_file(path)

@spec delete_file(str) -> Result[None, Error]

Deletes a single file or an empty directory at the specified path.

Returns Ok(None) if successful, or Err(reason) if an error occurs.

This function attempts to remove both files and empty directories without first checking the type of the path. It tries to remove the path as a file first, and if that fails, it attempts to remove it as a directory.

Possible (but not limited to) errors:

  • FileNotFound: The file or directory does not exist
  • PermissionDenied: Lack of permission to delete the file or directory
  • NotEmpty: Attempted to remove a non-empty directory
  • OSError: Other OS-level errors (e.g., I/O error)

Examples

# Delete an existing file
result = delete_file("example.txt")
# Ok(None)
# Delete an empty directory
result = delete_file("empty_dir")
# Ok(None)
# Try to delete a non-existent file or directory
result = delete_file("nonexistent.txt")
# Err(FileNotFound)
# Try to delete a non-empty directory
result = delete_file("my_directory")
# Err(NotEmpty)

2. delete_directory(path, recursive=False)

@spec delete_directory(str, bool) -> Result[None, Error]

Deletes a directory at the specified path. If recursive is True, it will delete the directory and all its contents.

Returns Ok(None) if successful, or Err(reason) if an error occurs.

Note: For non-recursive deletion of an empty directory, this function behaves the same as delete_file(path).

Possible (but not limited to) errors:

  • FileNotFound: The directory does not exist
  • PermissionDenied: Lack of permission to delete the directory or its contents
  • NotADirectory: The path is not a directory
  • OSError: Other OS-level errors (e.g., I/O error)

Examples

# Delete an empty directory
result = delete_directory("empty_dir")
# Ok(None)
# Try to delete a non-empty directory
result = delete_directory("non_empty_dir")
# Err(NotEmpty)
# Delete a directory and its contents
result = delete_directory("non_empty_dir", recursive=True)
# Ok(None)
# Try to delete a file using delete_directory
result = delete_directory("file.txt")
# Err(NotADirectory)

Considerations

  • Implement a single function for both file and empty directory removal, similar to go's approach. as it can be more efficient than separate functions.
  • Avoid using a separate stat call to determine if the path is a file or directory. Instead, attempt both file and directory removal operations, as this is generally more efficient.
  • Be aware of differences in error reporting, especially for operations like unlink and rmdir on files and directories.
  • Implement proper error handling and propagation.
  • Ensure that read-only files can be deleted (will probably need some additional sys calls).
  • Add appropriate logging for debugging.

Test cases to implement

  1. Delete a regular file
  2. Attempt to delete a non-existent file
  3. Delete an empty directory
  4. Attempt to delete a non-empty directory
  5. Attempt to delete a file without proper permissions
  6. Delete a non-empty directory (recursive)
  7. Delete a read-only file
  8. Attempt to delete a file that is currently in use by another process
@kings177 kings177 added this to the IO lib v0 milestone Aug 20, 2024
@developedby developedby changed the title File deletion Add IO functions for removing files and directories Aug 21, 2024
@imaqtkatt imaqtkatt self-assigned this Aug 22, 2024
@kings177 kings177 added the enhancement New feature or request label Aug 29, 2024
@developedby
Copy link
Member

@kings177 I don't think this is a good interface.

  • First, this operation is usually called "remove" and not "delete".
  • delete_file implies that it only removes non-directory files, which isn't the case. It'd be better to just call it delete or even better, remove.
  • delete_directory without passing the recursive flag is redundant with delete_file and in almost all cases useless. This function really only exists for its recursive option.
  • It's inconsistent for delete_file to remove both directories and non-directory files while delete_directory removes only directories. It'd be better for both functions to have similar behaviours.

I think a better interface would be one of these two:

  • remove for removing non-directory files and empty directories and remove_all for removing any files recursively, including trees of directories.
  • remove_file for removing non-directory files, remove_dir for removing empty directories, remove_tree for removing trees of directories.

Personally I'm favorable to the first one, but either would be fine.

The second one can still be implemented with only two functions and the recursive flag, choosing between recursive or not can be an additional layer in bend.

As always, names can still be bikeshed (remove vs remove_file, remove_dir vs rmdir, etc)

@kings177
Copy link
Member Author

those are some pretty good points, and i agree.

remove for removing non-directory files and empty directories and remove_all for removing any files recursively, including trees of directories.

i do prefer this over the one originally written in the issue.
so changes to this would be:

  1. instead of delete_file(path) we would have remove(path)
  2. instead of delete_directory(path) we would have remove_all(path)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants