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

Changes related to File Provider #30

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jayamala17
Copy link
Contributor

@jayamala17 jayamala17 commented Jan 21, 2025

Filesystem operations (e.g., copying, deleting, creating directories) will remain in the FileProvider since they directly interact with the file system. Currently, I have path manipulation operations (e.g., joining, normalizing paths) in FileProvider. Would it be better to move these to a separate utility class, or should they remain in FileProvider for easier maintainability?

Filesystem Operations in FileProvider

def clone_directory_structure(self, source_path: Path, destination_path: Path):
    shutil.copytree(source_path, destination_path)

def delete_dir_and_contents(self, path: Path):
    shutil.rmtree(path, ignore_errors=True)

def create_directory(self, path: Path):
    os.mkdir(path)

def create_directories(self, path: Path):
    os.makedirs(path)

Path ManipulationOperations in FileProvider

def apply_relative_path(self, base_path: Path, path_to_apply: str) -> Path:
    resolved_combined_path = self.get_norm_path(base_path, path_to_apply)
    final_path = resolved_combined_path.relative_to(base_path.parent)
    return final_path

def get_descriptor_dir(self, file_path: Path) -> Path:
    return file_path.parent

def get_norm_path(self, base_path: Path, path_to_apply: str) -> Path:
    combined_path = self.get_combined_path(base_path, path_to_apply)
    resolved_combined_path = Path(os.path.normpath(combined_path))
    return resolved_combined_path

def get_combined_path(self, base_path: Path, path_to_apply: str) -> Path:
    return Path(os.path.join(base_path, path_to_apply))

@jayamala17 jayamala17 marked this pull request as draft January 21, 2025 21:11
def clone_directory_structure(self, source_path: Path, destination_path: Path):
shutil.copytree(source_path, destination_path)

def get_environment_variable(self, env_key: str, default_value: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayamala17 I may have missed this discussion, but why is the file provider responsible for environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the os.getenv calls to the FileProvider class because it was using the os module for file operations, and it seemed logical to centralize all related functionality in one place. However now I believe that environment variable handling should be separated from file operations.

@jayamala17 jayamala17 marked this pull request as ready for review January 22, 2025 19:19
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.

3 participants