Ignore fs.WalkDir errors in FindFiles

[parkerduckworth]

Updates FindFiles to use fs.WalkDir instead of filepath.Walk, and ignores any errors that occur during the search.

Closes #99

[bitfield]

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?

[parkerduckworth]

Yeah great point, pushed up a test

[bitfield]

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.

[kwilczynski]

Hi @parkerduckworth,

This new functionality does make a lot of sense from a pure UX point of view. However, it might ever so slightly violate the Principle of Least Surprise.

Why? You might ask. A number of reasons come to mind, for instance:

• Hides errors...
• ... which changes the existing behaviour of this function
• Does not allow you to alter the behaviour, neither enable nor disable the new behaviour
• Changes the assumption about empty results: was there an error (and if so, then what was it?), or the files or directories are simply missing?

The change we are adding claims that this new functionality aligns FindFiles() closer to how find (man 1 find) works by ignoring errors. However, one could argue that find does not ignore errors per se - it does not stop execution when it can't stat() (the system call) a file or directory; however, the errors are very much not ignored, as errors are printed to the standard error output and should there be even a single I/O error, then find would return a non-zero exit code once it finishes to let you know that there was something wrong during its execution. Additionally, find can return an error without any results, should the provided path be inaccessible, for example (perhaps not the best example):

This is something that countless people often rely on in their shell scripts. So, there is an API, and there is an interface between find and the user, of sorts, there. I, for one, would like to know that there was an error somewhere.

What do you think?

Perhaps a way forward here would be such where FindFiles() would collect errors it saw, so to speak, and then return an error type that carries details of multiple errors (see the hashicorp/go-multierror package). This would allow for the Error() function on the Pipe type (the fluent interface of Script that makes it so nice to use can also be a design challenge) to work as usual (since the error interface is still the one we return), and users would be able to reason about any potential errors.

A different approach would be to perhaps introduce a new function called Find(), which takes one argument that denotes the type of what to filter (akin to -type f or -type d when using find), and two other arguments - an error and results callbacks. Or perhaps only two arguments - a type and a callback (similarly to the existing Filter() function).

@bitfield, your thoughts? @parkerduckworth yours?

Thank you!

[bitfield]

Maybe a sensible compromise here is that:

1. If FindFiles has any results, there is no error
2. If FindFiles has no results, but there was at least one error, then that error becomes the pipe's error status.

What do you think?