Open

Ignore fs.WalkDir errors in FindFiles #122

Show file tree
Hide file tree
Changes from all commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f21f9d5
ignore fs.WalkDir errs in FindFiles
parkerduckworthJun 2, 2022
640a46b
add error case to FindFiles tests
parkerduckworthJun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Failed to load files.
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@ import (
"encoding/json"
"fmt"
"io"
"io/fs"
"os"
"os/exec"
"path/filepath"
Expand DownExpand Up@@ -219,20 +220,25 @@ func File(name string) *Pipe {
//
// test/1.txt
// test/2.txt
func FindFiles(path string) *Pipe {
func FindFiles(root string) *Pipe {
var fileNames []string
walkFn := func(path string, info os.FileInfo, err error) error {
walkFn := func(path string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
if !info.IsDir() {
fileNames = append(fileNames, path)
if !d.IsDir() {
fileNames = append(fileNames, filepath.Join(root, path))
}
return nil
}
if err := filepath.Walk(path, walkFn); err != nil {
return NewPipe().WithError(err)
}

// ignoring any errors here to more closely align
// behavior with `find`. for example, we wouldn't
// expect `find` to return an error without results
// if it encountered any permissions errors during
// its execution. see [ issue #99]
// (https://.com/bitfield/script/issues/99)
_ = fs.WalkDir(os.DirFS(root), ".", walkFn)
return Slice(fileNames)
}

Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -1017,11 +1017,22 @@ func TestFindFiles_RecursesIntoSubdirectories(t *testing.T) {
}
}

func TestFindFiles_InNonexistentPathReturnsError(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

If it's a key part of FindFiles behaviour that it doesn't return an error for missing paths, shouldn't we have a test for that?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah great point, pushed up a test

// FindFiles has been updated to ignore all errors
// see [ issue #99]
// (https://.com/bitfield/script/issues/99)
func TestFindFiles_FailsSilently(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think "fails silently" could be misinterpreted, couldn't it? Like "fails" suggests it's not working properly. What about something like:

Suggested change
func TestFindFiles_FailsSilently(t *testing.T) {
func TestFindFiles_ReturnsEmptyPipeAndNoErrorForNonexistentPath(t *testing.T) {

But I'm not convinced this is the right behaviour, actually. I am convinced that if there's some error from fs.WalkDir's traversal, we should ignore that. But if the user has asked us to FindFiles starting from some root that doesn't exist, I think that should be an error. Would you want to know that, as the programmer? I would.

t.Parallel()
p := script.FindFiles("nonexistent_path")
if p.Error() == nil {
t.Fatal("want error for nonexistent path")
want := "\n"
p := script.FindFiles("testdata/nonexistent")
if p.Error() != nil {
t.Fatal(p.Error())
}
got, err := p.String()
if err != nil {
t.Fatal(err)
}
if want != got {
t.Error(cmp.Diff(want, got))
}
}

Expand Down