Skip to content
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.

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

Ignore fs.WalkDir errors in FindFiles #122

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 13 additions & 7 deletions script.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"encoding/json"
"fmt"
"io"
"io/fs"
"os"
"os/exec"
"path/filepath"
Expand Down Expand 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 [github issue #99]
// (https://github.com/bitfield/script/issues/99)
_ = fs.WalkDir(os.DirFS(root), ".", walkFn)
return Slice(fileNames)
}

Expand Down
19 changes: 15 additions & 4 deletions script_test.go
Original file line number Diff line number Diff 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 [github issue #99]
// (https://github.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