refactor copy files method. if there is a top level directory, remove it but keep the structure inside - #174
Conversation
… it but keep the structure inside
Walkthrough
ChangesFilesystem Copy Behavior
Sequence Diagram(s)sequenceDiagram
participant DLUFileHandler
participant SourceFilesystem
participant DestinationFilesystem
DLUFileHandler->>SourceFilesystem: Resolve package and fallback source paths
DLUFileHandler->>DestinationFilesystem: Clear destination package directory
DLUFileHandler->>SourceFilesystem: Read files and directory contents
DLUFileHandler->>DestinationFilesystem: Create directories and copy missing files
DLUFileHandler-->>DLUFileHandler: Return files_copied
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if os.path.exists(base_dest_package_directory): | ||
| logger.info( | ||
| "Removing existing destination directory %s", | ||
| base_dest_package_directory, | ||
| ) | ||
| shutil.rmtree(base_dest_package_directory) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not destroy the existing package before the replacement succeeds.
The destination is removed before validating all sources. An empty file_list, missing source, or mid-copy failure leaves the prior package deleted or partially rebuilt.
Copy into a temporary sibling directory and replace the destination only after the entire operation succeeds.
| if preserve_path: | ||
| dest_package_directory = os.path.join(dest_package_directory, | ||
| file.get_short_path()) | ||
|
|
||
| subdirs = [os.path.join(source_package_directory, o) | ||
| for o in os.listdir(source_package_directory) | ||
| if os.path.isdir(os.path.join(source_package_directory, o))] | ||
| dir = "".join(subdirs) | ||
| if len(os.listdir(source_package_directory)) == 1 and os.path.isdir(source_package_directory) and os.path.isdir(dir): | ||
| os.chdir(dir) | ||
| allfiles = os.listdir(dir) | ||
| for f in allfiles: | ||
| src_path = os.path.join(dir, f) | ||
| dst_path = os.path.join(dest_package_directory, f) | ||
| if not os.path.isdir(dest_package_directory): | ||
| os.mkdir(dest_package_directory) | ||
| if os.path.isfile(f): | ||
| logger.info("Copying file " + f + " to " + dst_path) | ||
| shutil.copy(src_path, dst_path) | ||
| files_copied += 1 | ||
| short_path = file.get_short_path() | ||
|
|
||
| if short_path: | ||
| dest_package_directory = os.path.join( | ||
| dest_package_directory, | ||
| short_path, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Flatten the wrapper into the base destination, not each file’s preserved path.
With preserve_path=True, dest_package_directory includes file.get_short_path(). Copying the complete wrapper there duplicates the path hierarchy and may copy the tree repeatedly for different files.
Proposed fix
+ wrapper_destination = base_dest_package_directory
wrapper_key = (
os.path.abspath(only_item_path),
- os.path.abspath(dest_package_directory),
+ os.path.abspath(wrapper_destination),
)
...
files_copied += self.copy_directory_contents(
src_dir=only_item_path,
- dst_dir=dest_package_directory,
+ dst_dir=wrapper_destination,
)Also applies to: 340-358
| try: | ||
| top_level_items = os.listdir(source_package_directory) | ||
| except FileNotFoundError: | ||
| raise FileNotFoundError( | ||
| f"Cannot list source package directory because it does not exist: " | ||
| f"{source_package_directory}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the original FileNotFoundError as the exception cause.
Proposed fix
- except FileNotFoundError:
+ except FileNotFoundError as err:
raise FileNotFoundError(
f"Cannot list source package directory because it does not exist: "
f"{source_package_directory}"
- )
+ ) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| top_level_items = os.listdir(source_package_directory) | |
| except FileNotFoundError: | |
| raise FileNotFoundError( | |
| f"Cannot list source package directory because it does not exist: " | |
| f"{source_package_directory}" | |
| ) | |
| try: | |
| top_level_items = os.listdir(source_package_directory) | |
| except FileNotFoundError as err: | |
| raise FileNotFoundError( | |
| f"Cannot list source package directory because it does not exist: " | |
| f"{source_package_directory}" | |
| ) from err |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 330-333: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
Source: Linters/SAST tools
2 participants
Summary by CodeRabbit
Bug Fixes
New Features