add in skip handler - #20
Conversation
pr-insights
Bot
added
VH/complexity
There was a problem hiding this comment.
Pull request overview
This pull request adds a skip handler feature to the gocodewalker library, allowing users to register a callback function that is invoked whenever a file or directory is skipped during the walking process. The skip handler receives the path, name, directory flag, and reason for the skip, enabling users to track or log which files/directories are being filtered out and why.
Changes:
- Added a new
SkipReasontype with 15 constants representing different skip reasons (gitignore, binary, hidden, extension filters, etc.) - Added
skipHandlerfield toFileWalkerandSetSkipHandler()method to register custom skip handlers with a default no-op implementation - Integrated skip reason tracking and handler invocation throughout the file filtering logic in
walkDirectoryRecursive() - Added comprehensive test coverage with 5 new test functions covering various skip scenarios for both files and directories
- Updated documentation in README.md with examples of skip handler usage
- Added skip handler example to the main.go demo program
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| file.go | Implements skip handler infrastructure: defines SkipReason type and constants, adds skipHandler field and SetSkipHandler method, instruments all skip logic to track reasons and invoke handler |
| file_test.go | Adds comprehensive test suite with 368 lines covering default no-op behavior, nil handler, file skip reasons, and directory skip reasons |
| cmd/gocodewalker/main.go | Demonstrates skip handler usage in example program and changes walker paths from specific directories to current directory |
| README.md | Documents skip handler feature with usage example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestSkipHandlerFileCases(t *testing.T) { | ||
| type skipRecord struct { | ||
| path string | ||
| name string | ||
| isDir bool | ||
| reason SkipReason | ||
| } | ||
|
|
||
| type testcase struct { | ||
| Name string | ||
| Setup func() (*FileWalker, chan *File) | ||
| ExpectedSkips int | ||
| ExpectedReason SkipReason | ||
| ExpectedIsDir bool | ||
| } | ||
|
|
||
| testCases := []testcase{ | ||
| { | ||
| Name: "ExcludeFilename skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "excluded.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeFilename = []string{"excluded.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeFilename, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "IncludeFilename skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "other.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IncludeFilename = []string{"wanted.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonIncludeFilename, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "ExcludeFilenameRegex skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.log")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeFilenameRegex = []*regexp.Regexp{regexp.MustCompile(`\.log$`)} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeFilenameRegex, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "IncludeFilenameRegex skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IncludeFilenameRegex = []*regexp.Regexp{regexp.MustCompile(`\.go$`)} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonIncludeFilenameRegex, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "AllowListExtension skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.md")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.AllowListExtensions = []string{"go"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonAllowListExtension, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "ExcludeListExtension skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeListExtensions = []string{"txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeListExtension, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "LocationExcludePattern file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.LocationExcludePattern = []string{"test.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonLocationExcludePattern, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "Binary file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _ = os.WriteFile(filepath.Join(d, "binary.bin"), []byte{0}, 0644) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IgnoreBinaryFiles = true | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonBinary, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "CustomIgnorePatterns file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.md")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.CustomIgnorePatterns = []string{"*.md"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonCustomIgnore, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.Name, func(t *testing.T) { | ||
| walker, fileListQueue := tc.Setup() | ||
|
|
||
| var skips []skipRecord | ||
| walker.SetSkipHandler(func(path string, name string, isDir bool, reason SkipReason) { | ||
| skips = append(skips, skipRecord{path: path, name: name, isDir: isDir, reason: reason}) | ||
| }) | ||
| walker.osReadFile = func(name string) ([]byte, error) { return nil, nil } | ||
| _ = walker.Start() | ||
|
|
||
| // drain channel | ||
| for range fileListQueue { | ||
| } | ||
|
|
||
| if len(skips) != tc.ExpectedSkips { | ||
| t.Errorf("expected %d skips but got %d", tc.ExpectedSkips, len(skips)) | ||
| return | ||
| } | ||
|
|
||
| if len(skips) > 0 { | ||
| if skips[0].reason != tc.ExpectedReason { | ||
| t.Errorf("expected reason %q but got %q", tc.ExpectedReason, skips[0].reason) | ||
| } | ||
| if skips[0].isDir != tc.ExpectedIsDir { | ||
| t.Errorf("expected isDir=%v but got isDir=%v", tc.ExpectedIsDir, skips[0].isDir) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for SkipReasonGitignore. The code at lines 467-471 and 637-641 adds this skip reason for gitignore patterns, but there are no tests in the TestSkipHandlerFileCases or TestSkipHandlerDirectoryCases test suites that verify this reason is correctly reported to the skip handler.
| func TestSkipHandlerFileCases(t *testing.T) { | ||
| type skipRecord struct { | ||
| path string | ||
| name string | ||
| isDir bool | ||
| reason SkipReason | ||
| } | ||
|
|
||
| type testcase struct { | ||
| Name string | ||
| Setup func() (*FileWalker, chan *File) | ||
| ExpectedSkips int | ||
| ExpectedReason SkipReason | ||
| ExpectedIsDir bool | ||
| } | ||
|
|
||
| testCases := []testcase{ | ||
| { | ||
| Name: "ExcludeFilename skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "excluded.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeFilename = []string{"excluded.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeFilename, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "IncludeFilename skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "other.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IncludeFilename = []string{"wanted.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonIncludeFilename, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "ExcludeFilenameRegex skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.log")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeFilenameRegex = []*regexp.Regexp{regexp.MustCompile(`\.log$`)} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeFilenameRegex, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "IncludeFilenameRegex skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IncludeFilenameRegex = []*regexp.Regexp{regexp.MustCompile(`\.go$`)} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonIncludeFilenameRegex, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "AllowListExtension skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.md")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.AllowListExtensions = []string{"go"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonAllowListExtension, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "ExcludeListExtension skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.ExcludeListExtensions = []string{"txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonExcludeListExtension, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "LocationExcludePattern file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.txt")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.LocationExcludePattern = []string{"test.txt"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonLocationExcludePattern, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "Binary file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _ = os.WriteFile(filepath.Join(d, "binary.bin"), []byte{0}, 0644) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.IgnoreBinaryFiles = true | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonBinary, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| { | ||
| Name: "CustomIgnorePatterns file skip", | ||
| Setup: func() (*FileWalker, chan *File) { | ||
| d, _ := os.MkdirTemp(os.TempDir(), randSeq(10)) | ||
| _, _ = os.Create(filepath.Join(d, "test.md")) | ||
|
|
||
| fileListQueue := make(chan *File, 10) | ||
| walker := NewFileWalker(d, fileListQueue) | ||
| walker.CustomIgnorePatterns = []string{"*.md"} | ||
| return walker, fileListQueue | ||
| }, | ||
| ExpectedSkips: 1, | ||
| ExpectedReason: SkipReasonCustomIgnore, | ||
| ExpectedIsDir: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.Name, func(t *testing.T) { | ||
| walker, fileListQueue := tc.Setup() | ||
|
|
||
| var skips []skipRecord | ||
| walker.SetSkipHandler(func(path string, name string, isDir bool, reason SkipReason) { | ||
| skips = append(skips, skipRecord{path: path, name: name, isDir: isDir, reason: reason}) | ||
| }) | ||
| walker.osReadFile = func(name string) ([]byte, error) { return nil, nil } | ||
| _ = walker.Start() | ||
|
|
||
| // drain channel | ||
| for range fileListQueue { | ||
| } | ||
|
|
||
| if len(skips) != tc.ExpectedSkips { | ||
| t.Errorf("expected %d skips but got %d", tc.ExpectedSkips, len(skips)) | ||
| return | ||
| } | ||
|
|
||
| if len(skips) > 0 { | ||
| if skips[0].reason != tc.ExpectedReason { | ||
| t.Errorf("expected reason %q but got %q", tc.ExpectedReason, skips[0].reason) | ||
| } | ||
| if skips[0].isDir != tc.ExpectedIsDir { | ||
| t.Errorf("expected isDir=%v but got isDir=%v", tc.ExpectedIsDir, skips[0].isDir) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for SkipReasonIgnoreFile. The code at lines 477-481 and 646-650 adds this skip reason for ignore file patterns, but there are no tests in the TestSkipHandlerFileCases or TestSkipHandlerDirectoryCases test suites that verify this reason is correctly reported to the skip handler.
| func main() { | ||
| fileListQueue := make(chan *gocodewalker.File, 10_000) | ||
| fileWalker := gocodewalker.NewParallelFileWalker([]string{"./cmd/", "./go-gitignore/"}, fileListQueue) | ||
| fileWalker := gocodewalker.NewParallelFileWalker([]string{"."}, fileListQueue) |
There was a problem hiding this comment.
The path for NewParallelFileWalker was changed from scanning specific directories (./cmd/, ./go-gitignore/) to scanning the entire current directory (.). This change may have been made for testing the skip handler but appears unrelated to the skip handler feature itself. If this change was unintentional or only for testing purposes, it should be reverted to preserve the original functionality of this example program.
| fileWalker := gocodewalker.NewParallelFileWalker([]string{"."}, fileListQueue) | |
| fileWalker := gocodewalker.NewParallelFileWalker([]string{"./cmd/", "./go-gitignore/"}, fileListQueue) |
| if ignore.MatchIsDir(joined, false) != nil { | ||
| shouldIgnore = ignore.Ignore(joined) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonCustomIgnore | ||
| } | ||
| } |
There was a problem hiding this comment.
The skipReason could be incorrectly set when ignore.Ignore returns false. When shouldIgnore is set to false by ignore.Ignore at line 488, the skipReason retains its previous value. If a subsequent filter check then sets shouldIgnore to true, the skipReason might incorrectly reference an earlier custom ignore rule instead of the actual reason for skipping. Consider only setting skipReason when shouldIgnore transitions from false to true, or clear skipReason when shouldIgnore becomes false.
| if ignore.MatchIsDir(joined, true) != nil { | ||
| shouldIgnore = ignore.Ignore(joined) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonGitignore | ||
| } | ||
| } |
There was a problem hiding this comment.
The skipReason could be incorrectly set when ignore.Ignore returns false. When shouldIgnore is set to false by ignore.Ignore at line 638, the skipReason retains its previous value. If a subsequent filter check then sets shouldIgnore to true, the skipReason might incorrectly reference an earlier gitignore rule instead of the actual reason for skipping. Consider only setting skipReason when shouldIgnore transitions from false to true, or clear skipReason when shouldIgnore becomes false.
| if len(f.IncludeDirectory) != 0 { | ||
| shouldIgnore = !slices.ContainsFunc(f.IncludeDirectory, func(allow string) bool { | ||
| return dir.Name() == allow | ||
| }) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonIncludeDirectory | ||
| } | ||
| } |
There was a problem hiding this comment.
The IncludeDirectory filter unconditionally overwrites shouldIgnore, which can incorrectly un-skip directories that were already marked for skipping by earlier filters (gitignore, ignore files, custom ignores, or module ignores). If a directory was marked shouldIgnore=true by a previous filter, this check will set shouldIgnore=false if the directory matches IncludeDirectory, effectively undoing the earlier skip decision. This behavior may be intentional, but the skipReason won't be cleared, leading to incorrect reason reporting.
| if ignore.MatchIsDir(joined, false) != nil { | ||
| shouldIgnore = ignore.Ignore(joined) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonIgnoreFile | ||
| } | ||
| } |
There was a problem hiding this comment.
The skipReason could be incorrectly set when ignore.Ignore returns false. When shouldIgnore is set to false by ignore.Ignore at line 478, the skipReason retains its previous value. If a subsequent filter check then sets shouldIgnore to true, the skipReason might incorrectly reference an earlier ignore file rule instead of the actual reason for skipping. Consider only setting skipReason when shouldIgnore transitions from false to true, or clear skipReason when shouldIgnore becomes false.
|
|
||
| ### Skip Handler | ||
|
|
||
| You can supply your own skip handler when walking. This allows you to perform an action a file is skipped. |
There was a problem hiding this comment.
Grammatical error: missing "when" between "action" and "a file". Should read: "This allows you to perform an action when a file is skipped."
| You can supply your own skip handler when walking. This allows you to perform an action a file is skipped. | |
| You can supply your own skip handler when walking. This allows you to perform an action when a file is skipped. |
| if len(f.IncludeDirectoryRegex) != 0 { | ||
| shouldIgnore = !slices.ContainsFunc(f.IncludeDirectoryRegex, func(allow *regexp.Regexp) bool { | ||
| return allow.MatchString(dir.Name()) | ||
| }) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonIncludeDirectoryRegex | ||
| } | ||
| } |
There was a problem hiding this comment.
The IncludeDirectoryRegex filter unconditionally overwrites shouldIgnore, which can incorrectly un-skip directories that were already marked for skipping by earlier filters. If a directory was marked shouldIgnore=true by a previous filter, this check will set shouldIgnore=false if the directory matches IncludeDirectoryRegex, effectively undoing the earlier skip decision. This behavior may be intentional, but the skipReason won't be cleared, leading to incorrect reason reporting.
| if ignore.MatchIsDir(joined, false) != nil { | ||
| shouldIgnore = ignore.Ignore(joined) | ||
| if shouldIgnore { | ||
| skipReason = SkipReasonGitignore | ||
| } | ||
| } |
There was a problem hiding this comment.
The skipReason could be incorrectly set when ignore.Ignore returns false. When shouldIgnore is set to false by ignore.Ignore at line 468, the skipReason retains its previous value (or remains uninitialized). If a subsequent filter check then sets shouldIgnore to true, the skipReason might incorrectly reference an earlier gitignore rule instead of the actual reason for skipping. Consider only setting skipReason when shouldIgnore transitions from false to true, or clear skipReason when shouldIgnore becomes false.
No description provided.