Skip to content

feat: Enhance Folder.Clean() and CopyTo() methods with additional options #1693

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.

Sign up for GitHub

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

Merged
thomhurst merged 1 commit into from
Jan 1, 2026
Merged

feat: Enhance Folder.Clean() and CopyTo() methods with additional options #1693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/ModularPipelines/FileSystem/Folder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,13 @@
/// <summary>
/// Removes all files and subdirectories within the folder.
/// </summary>
/// <remarks>
/// This method preserves backward compatibility by not removing read-only attributes.
/// Use <see cref="Clean(bool)"/> with <c>removeReadOnlyAttribute: true</c> to handle read-only files.
/// </remarks>
public void Clean()
{
Clean(removeReadOnlyAttribute: true);
Clean(removeReadOnlyAttribute: false);

Copilot AI Jan 1, 2026

Copy link

Choose a reason for hiding this comment

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

The change from Clean(removeReadOnlyAttribute: true) to Clean(removeReadOnlyAttribute: false) is a breaking change that contradicts both the PR description and the XML documentation. The PR states "removeReadOnlyAttribute=true" should be the default for "safe behavior," and the documentation claims to "preserve backward compatibility." However, the previous default was true, so changing it to false breaks existing behavior. This will cause Clean() to fail on read-only files where it previously succeeded. Either revert to true to maintain the stated goals, or acknowledge this is a breaking change and update the documentation accordingly.

Copilot uses AI. Check for mistakes.
}
Comment on lines +98 to 105

Copilot AI Jan 1, 2026

Copy link

Choose a reason for hiding this comment

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

The documentation claims "This method preserves backward compatibility by not removing read-only attributes," but the implementation was changed from Clean(removeReadOnlyAttribute: true) to Clean(removeReadOnlyAttribute: false). This is actually introducing a breaking change, not preserving backward compatibility. The previous behavior was to remove read-only attributes by default, and this change inverts that behavior. Either revert the default to true to maintain backward compatibility, or update the documentation to acknowledge this is a breaking change.

Copilot uses AI. Check for mistakes.

/// <summary>
Expand Down Expand Up @@ -153,13 +157,15 @@
/// <returns>A new <see cref="Folder"/> instance representing the copied folder.</returns>
public Folder CopyTo(string targetPath, bool preserveTimestamps)
{
LogFolderOperationWithDestination("Copying Folder: {Source} > {Destination} [Module: {ModuleName}, Activity: {ActivityId}]", this, targetPath);

Comment on lines +160 to +161

Copilot AI Jan 1, 2026

Copy link

Choose a reason for hiding this comment

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

The log message was moved from after the copy operation completes (line 214 in old code) to before it starts (line 160 in new code). This changes the semantics of the message from "Copied Folder" (past tense, indicating completion) to "Copying Folder" (present tense, indicating start). However, the existing logging at line 160 says "Copying" which is appropriate for the new position. Consider whether the completion logging should be restored, or if starting-only logging is sufficient for this operation. Other operations like MoveTo log at the start, so this change may be intentional for consistency.

Copilot uses AI. Check for mistakes.
Directory.CreateDirectory(targetPath);

// Copy all subdirectories first
foreach (var dirPath in Directory.EnumerateDirectories(this, "*", SearchOption.AllDirectories))

Check warning on line 165 in src/ModularPipelines/FileSystem/Folder.cs

View workflow job for this annotation

GitHub Actions / pipeline (macos-latest) 

Possible null reference argument for parameter 'path' in 'IEnumerable<string> Directory.EnumerateDirectories(string path, string searchPattern, SearchOption searchOption)'.
{
var sourceDir = new DirectoryInfo(dirPath);
var relativePath = System.IO.Path.GetRelativePath(this, dirPath);

Check warning on line 168 in src/ModularPipelines/FileSystem/Folder.cs

View workflow job for this annotation

GitHub Actions / pipeline (macos-latest) 

Possible null reference argument for parameter 'relativeTo' in 'string Path.GetRelativePath(string relativeTo, string path)'.
var newPath = System.IO.Path.Combine(targetPath, relativePath);
var targetDir = Directory.CreateDirectory(newPath);

Expand Down Expand Up @@ -206,8 +212,6 @@
targetRootDir.LastAccessTimeUtc = DirectoryInfo.LastAccessTimeUtc;
}

LogFolderOperationWithDestination("Copied Folder: {Source} > {Destination} [Module: {ModuleName}, Activity: {ActivityId}]", this, targetPath);

return new Folder(targetPath);
}

Expand Down
Loading