feat: add dry-run support, OS Specific ignore file #13
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces dry-run support, enhances path normalization, and adds OS-specific default ignore patterns.
- Adds
dryRunflag across the linking workflow and updates tests to cover dry-run behavior. - Normalizes paths in
PathEqualsusingfilepath.Clean. - Introduces
defaultIgnorePatternsfor OS-specific and common backup files and enhances ignore handling.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/path_utils.go | Use filepath.Clean to normalize paths before comparing |
| utils/path_utils_test.go | Add tests for redundant elements, separators, and Windows cases |
| services/filelinker_service.go | Propagate dryRun flag, wire up default ignores, and log dry-run operations |
| services/file_ignore_enhanced.go | Implement enhanced ignore logic including negations |
| cmd/dotfileslinker/main.go | Parse --dry-run flag and log final dry-run status |
| README.md / README_ja.md | Document the new --dry-run option and update test/lint instructions |
| // Clean paths to normalize directory separators and remove redundant elements | ||
| cleanA := filepath.Clean(absA) | ||
| cleanB := filepath.Clean(absB) | ||
|
|
||
| // Perform case-sensitive comparison on all platforms |
There was a problem hiding this comment.
On Windows, path comparisons should be case-insensitive (including drive letters). Consider using strings.EqualFold(cleanA, cleanB) when runtime.GOOS == "windows".
| // Perform case-sensitive comparison on all platforms | |
| // Perform case-insensitive comparison on Windows and case-sensitive comparison on other platforms | |
| if runtime.GOOS == "windows" { | |
| return strings.EqualFold(cleanA, cleanB) | |
| } |
Copilot uses AI. Check for mistakes.
| name: "Windows paths with different drive letter casing", | ||
| pathA: "C:\\temp\\test", | ||
| pathB: "c:\\temp\\test", | ||
| expected: false, // Case sensitivity applies to drive letters too |
There was a problem hiding this comment.
Windows filesystem is case-insensitive for drive letters; this test should expect true or strings.EqualFold behavior instead of enforcing case sensitivity.
| expected: false, // Case sensitivity applies to drive letters too | |
| expected: true, // Windows drive letters are case-insensitive |
Copilot uses AI. Check for mistakes.
| if len(ignoredFiles) > 0 { | ||
| s.logger.Info(fmt.Sprintf("Ignoring %d files from %s directory based on ignore patterns:", len(ignoredFiles), srcDir)) | ||
| for _, file := range ignoredFiles { | ||
| s.logger.Verbose(fmt.Sprintf(" Ignored file: %s (matched ignore pattern)", file)) |
There was a problem hiding this comment.
[nitpick] Logging of ignored files is inconsistent with processRepositoryRoot (which logs only filepath.Base(file)). Consider using filepath.Base(file) here for uniform output.
| s.logger.Verbose(fmt.Sprintf(" Ignored file: %s (matched ignore pattern)", file)) | |
| s.logger.Verbose(fmt.Sprintf(" Ignored file: %s (matched ignore pattern)", filepath.Base(file))) |
Copilot uses AI. Check for mistakes.
| if dryRun { | ||
| logger.Success("Dry run completed successfully. No changes were made.") | ||
| } else { |
There was a problem hiding this comment.
[nitpick] Dry-run completion is already logged by the service (DRY RUN COMPLETED). Consider removing this duplicate message to avoid redundancy.
| if dryRun { | |
| logger.Success("Dry run completed successfully. No changes were made.") | |
| } else { | |
| if !dryRun { |
Copilot uses AI. Check for mistakes.
No description provided.