Skip to content

Improve chown implementation - Address code review recommendations #24

Description

@fbraza

Summary

This issue tracks improvements to the chown implementation based on code review recommendations from PR #23. The goal is to enhance code quality, consistency, and robustness.

High Priority Improvements

1. Type Safety - Replace null with Option[String]

Current:

def apply(path: String, owner: String, group: String)(implicit fs: FileSystem): Unit
// Usage: chown("/path", "owner", null)

Proposed:

def apply(path: String, owner: String, group: Option[String] = None)(implicit fs: FileSystem): Unit
// Usage: chown("/path", "owner", Some("group")) or chown("/path", "owner")

2. Input Validation

Current: No validation for null/empty strings
Proposed:

def apply(path: String, owner: String)(implicit fs: FileSystem): Unit = {
  require(path != null && path.nonEmpty, "Path cannot be null or empty")
  require(owner != null && owner.nonEmpty, "Owner cannot be null or empty")
  // ... rest of implementation
}

3. Enhanced Error Handling for chmod

Current:

object chmod {
  def apply(path: String, perm: FsPermission)(implicit fs: FileSystem): Unit = {
    val pathToSet = new Path(path)
    fs.setPermission(pathToSet, perm)
  }
}

Proposed:

object chmod {
  def apply(path: String, perm: FsPermission)(implicit fs: FileSystem): Unit = {
    val pathToSet = new Path(path)
    try {
      if (!fs.exists(pathToSet)) {
        throw new FileNotFoundException(s"Path does not exist: $path")
      }
      fs.setPermission(pathToSet, perm)
    } catch {
      case e: FileNotFoundException => throw e
      case e: IOException => throw new IOException(s"Failed to change permissions for $path: ${e.getMessage}", e)
    }
  }
}

Medium Priority Improvements

4. Additional Test Coverage

Add tests for:

  • Null/empty owner and group strings
  • Invalid user/group name formats
  • Permission denied scenarios
  • Symbolic link handling
  • Very deep directory structures

5. Complete Documentation

  • Complete Perm object documentation
  • Add usage examples to Scaladoc
  • Document error scenarios and recovery

6. User/Group Name Format Validation

private def isValidUserName(name: String): Boolean = {
  name != null && name.matches("^[a-zA-Z0-9._-]+$")
}

Low Priority Improvements

7. Performance Optimizations

  • Consider adding batching for large directory operations
  • Add configuration options for recursion limits
  • Progress reporting for large operations

8. Enhanced API Features

  • Dry-run mode
  • Follow symlinks option
  • Verbose logging option

Acceptance Criteria

  • Replace null with Option[String] for optional group parameter
  • Add input validation for all parameters
  • Implement proper error handling in chmod object
  • Add comprehensive test coverage for edge cases
  • Complete all Scaladoc documentation
  • All existing tests continue to pass

Files to Modify

  • src/main/scala/permOps.scala - Main implementation improvements
  • src/test/scala/permOps.scala - Additional test cases

Related Issues

🤖 Generated with Claude Code

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions